Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt using platform specifier in path for web extensions #180525

Closed
sandy081 opened this issue Apr 21, 2023 · 7 comments
Closed

Adopt using platform specifier in path for web extensions #180525

sandy081 opened this issue Apr 21, 2023 · 7 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug extension-host Extension host issues verified Verification succeeded web Issues related to running VSCode in the web
Milestone

Comments

@sandy081
Copy link
Member

sandy081 commented Apr 21, 2023

VS Code for the Web is not loading some resources in the extension with web target platform

Eg: https://ms-toolsai.vscode-unpkg.net/ms-toolsai/jupyter/2023.4.1011131224/extension/out/extension.web.bundle.js?target%3Dweb.

This seems to work fine with a simple extension sandy081.999-pse published with web target platform. I do not know why it is breaking for above extension path.

When target platform web is introduced, we decided to go with the query parameter route considering that extensions will handle URIs correctly given the vscode.uri util. It seems we need to make changes how extension paths are handled to fix this.

CC @joaomoreno

@sandy081 sandy081 added bug Issue identified by VS Code Team member as probable bug extensions Issues concerning extensions labels Apr 21, 2023
@sandy081 sandy081 added this to the May 2023 milestone Apr 21, 2023
@sandy081 sandy081 self-assigned this Apr 21, 2023
@sandy081
Copy link
Member Author

@DonJayamanne Can you please share the extension that is not working for you when you published PSE for Web target? I have one with web target platform and it works for me -sandy081.999-pse

@sandy081 sandy081 added extension-host Extension host issues web Issues related to running VSCode in the web labels Apr 21, 2023
@sandy081 sandy081 removed this from the May 2023 milestone Apr 21, 2023
@sandy081 sandy081 removed the extensions Issues concerning extensions label Apr 21, 2023
@sandy081 sandy081 assigned jrieken and unassigned jrieken and sandy081 Apr 21, 2023
@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Apr 21, 2023
@jrieken
Copy link
Member

jrieken commented Apr 21, 2023

When target platform web is introduced, we decided to go with the query parameter route considering that extensions will handle URIs correctly given the vscode.uri util.

I would suggest to reconsider this given that we didn't really think about this. We now discovered one issue, there is very likely more in our code base, and assuming all extensions use the uri utils is very bold

@jrieken jrieken changed the title VS Code for the Web is not loading an extension with web target platform Extensions with web target platform are associated with the nullExtension Apr 21, 2023
@DonJayamanne
Copy link
Contributor

DonJayamanne commented Apr 24, 2023

Can you please share the extension that is not working for you when you pu

@sandy081
Here it is

Same version here #180525 (comment)

Version 2023.4.1011131224 from the marketplace

Screenshot 2023-04-24 at 11 43 22

@joaomoreno
Copy link
Member

joaomoreno commented Apr 25, 2023

For context, @jrieken says:

Ok, we then need to make changes how extension paths are handled. Today /foo/bar/baz/hello.txt?something isn’t considered a child of /foo/bar/?something and therefore all those extensions run as “null extensions”. that doesn’t just affect proposed API but also logging, telemetry, storage, mementos etc pp (edited)

We construct a trie data structure for extension path lookups. For that it is just a constructor option and it ignores query and fragment. However, there might more issues because I assume this is reflected all the way into the Extension.extensionUri API and I don’t know if all extensions are expecting a query part (edited)

@joaomoreno
Copy link
Member

joaomoreno commented Apr 25, 2023

Two paths forward:

  • We push towards a fix in VSCode, by having the trie not bail out on query params. But as @jrieken mentioned, we might find other places in the product that do not play well with query params in URIs, further down the flow.
  • We allow using the path component to specify platforms in vscode-unpkg.

By now it's rather clear that the latter is the lowest hanging and lowest risk change.

@joaomoreno joaomoreno added this to the May 2023 milestone Apr 25, 2023
@joaomoreno joaomoreno removed the under-discussion Issue is under discussion for relevance, priority, approach label Apr 25, 2023
@joaomoreno joaomoreno changed the title Extensions with web target platform are associated with the nullExtension Adopt using platform specifier in path for web extensions Apr 25, 2023
@sandy081
Copy link
Member Author

We allow using the path component to specify platforms in vscode-unpkg.

With this we would also need migration on the client side - to adopt existing installed extensions to use the new path.

@joaomoreno
Copy link
Member

This is why I renamed and left this issue open. 😉

sandy081 added a commit that referenced this issue May 10, 2023
#180525 adopt to web platform specifier
@mjbvz mjbvz added the verified Verification succeeded label Jun 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug extension-host Extension host issues verified Verification succeeded web Issues related to running VSCode in the web
Projects
None yet
Development

No branches or pull requests

5 participants