Support TS server returning custom schemes

[arcanis]

We have a TS server that can return zip paths (zip:///path/to/module.zip/package.json). We also have a VSCode extension that can handle such paths using a zip: protocol handler.

Using "Go to definition" unfortunately doesn't work because VSCode hardcodes the list of valid protocols, so the zip: protocol is lost as soon as the service client reworks the path.

Looking at the blame history, it looks like the hardcode occurred organically, with the first one being a one-of check, later followed by another one-of workaround. I believe at this time VSCode didn't support custom schemes (at least not visible for extensions), so this made sense. Now, I think it should simply forward all schemes it recognizes. Would you accept a PR to this effect?

Related: #59650
Cc: @mjbvz @cspotcode

[mjbvz]

Moving upstream as this will require coordination between TS Server and VS Code

Historically, TS server crashes if you send it a document with a weird scheme that it can't handle so we need to make sure the plugin that handles zip schema is active before sending those files over from VS Code

[arcanis]

@mjbvz isn't the "vscode -> TS" direction the issue in #59650, whereas "TS -> vscode" is this one?

[mjbvz]

VS Code can't send over zip: files until the TS server tells us it understands them and won't crash (i.e. your plugin is running)

[arcanis]

But why would VSCode send a zip: path to TS? This issue is about VSCode opening the buffer from the zip: path sent by the TS server.

[mjbvz]

Won’t that file reference other files that are also under the zip path?

[cspotcode]

In our use-case, the zip paths always originate from the TS server, not VSCode.

• User opens /project/index.ts in VSCode.
• Code refers to a third-party dependency, for example: import {each} from 'lodash';
• Our TSServer with our plugin is able to resolve the dependency to a zip path, and to read the contents of the zip archive.
• When the user hits "go to definition," the server sends the zip path to VSCode. VSCode attempts to open the zip path. We need some way for VSCode to read zip archives, perhaps via a zip:// protocol handler.
• VSCode asks tsserver for semantic diagnostics, passing the same zip path to tsserver. It recognizes the zip path, because it originated from tsserver.

In the above timeline, VSCode is sending zip paths to TSServer, but only after those zip paths originated from TSServer, so it all works.

I'm still not sure what format these paths should be in. Today, yarn does zip paths kinda like this: /project/.yarn/cache/lodash.zip/node_modules/lodash/index.d.ts
But if we want to use a zip:// protocol handler in VSCode, then it'll need to see paths that are actually URIs and look like this: zip:///project/.yarn/cache/lodash.zip/node_modules/lodash/index.d.ts
We can do this translation within our custom TSServer implementation if necessary; we've already prototyped it. But we need VSCode to allow us to send it URIs, however is the best way to do that.

EDIT: sorry, I might be polluting the thread; sounds like @arcanis's explanation covers it.

[arcanis]

Won’t that file reference other files that are also under the zip path?

Yep, but if I understand correctly I think sending schemed references to VSCode is a better fit for the thread over at #59650 - this one is purely to receive them (both are needed for a perfect integration, but they are independent enough that they can be implemented separately).

Basically, for #59650 to be fixed TS will have to support custom schemes, but for the issue described in this thread to be fixed, changes are only needed on the VSCode side.

[mjbvz]

Thanks for the details. A few potential options:

• Add way for TS tell us which schemes to sync on start. I'd prefer this approach.

• Trust all files that come from TS server. This gets complicated as we need to dynamically enable/disable language features on the VS Code side. I'm also not sure off the top of my head how to handle server restarts (with the plugin being disabled after the restart)

[arcanis]

Why would language features have to be disabled or enabled on the VSCode side? I'm probably missing a piece, but for example in the case I mentioned the Zip extension provides the ambient scheme through the FileSystemProvider API, so it doesn't need to be enabled at runtime.

[arcanis]

Ping @mjbvz? From my outsider's perspective, the following addition (the highlighted part, added here) would completely solve this issue - am I missing something?

[arcanis]

Ping? Is this issue still "Needs more info"?

[mjbvz]

VS Code will have to maintain this fix and I'm concerned about it—as small a code change as it may be—causing problems down the road.

You need to prove not just that the change fixes for the feature request, but that it won't cause additional problems. Basically, apply the fix and then try breaking things. Go through a thorough test pass of all different states the editor could be in: having the extension enabled/disabled, files opened in different states, using different TS server versions, and so on. Test if as if you were the person who will have to maintain the code going forward and deal with every bug report related to JS/TS that comes in

Report the scenarios you tested, what works and what doesn't, and then we can go from there

[elmpp]

Please find below details for a matrix of scenarios that should hopefully add to the confidence levels of this proposed change.

To run the procedures:

• git clone --depth 1 git@github.com:elmpp/berry.git -b vscode-case-study yarn2-36943 && cd yarn2-36943
• (cd packages/vscode-zipfs && yarn build || true >& /dev/null && yarn vsce package)
• ./scripts/vscode-zip-test-procedure.sh ./vscode-case-study // vscode will build the first time so likely take few mins
• follow the steps

Matrix:
Typescript version: 2.7.1, 2.9, 3.5.1, 3.8, 3.8-pnpify
ZipFS extension: enabled, disabled
Package Manager: npm, yarn2
Package Arrangement: pnp, node_modules
Editor states (with '.ts'): default view, hover/cmd+click import package reference, hover/cmd+click/cmd+doubleclick import symbol reference,
Editor states (with '.d.ts'): default view, hover on symbol

Note all these scenarios are with the proposed change.

ping @mjbvz

[mjbvz]

Thanks.

Can you also test the case where I have a file opened that relies on your plugin, but I then reload VS Code so that the file is loaded again but with the plugin in a disabled or crashed ? I want to make sure we handle the case when TS server is sent a schema that it doesn't understand

[elmpp]

@mjbvz Extra test case added for disabled extension and reload

[mjbvz]

Should be fixed by #94986 I believe. I only verified the change did not break the normal VS Code JS/TS support.

@elmpp or @cspotcode Can you please verify that the fix also addresses the original problem you were running into?

[JacksonKearl]

Ah, bot doesn't know what commit closed this so it doesn't know when it should be released.