Adds support for zip archives

[arcanis]

This PR fixes yarnpkg/berry#1157. It also helps with #59650.

Thanks to #94986 we can open VSCode-supported virtual paths when returned by VSCode. However, we still can't send virtual paths to TS. As a result, while we can jump into a zip archive, we can't jump from there to other files.

My diff is a WIP aiming to make that possible. It's working, but I need input from @mjbvz and team to decide how you wish to proceed regarding the configuration part: since TS doesn't support virtual protocols out of the box, and crashes when provided one, we need a way to keep this behaviour gated.

My suggestion

I plan to implement a typescript.schemes configuration variable that, when set, adds a set of additional schemes on top of the ones defined in isSupportedScheme. This setting will only be taken into account if the user has also set a typescript.tsdk variable AND if the local SDK is enabled. By doing this we ensure that such schemes will only ever be active on TypeScript instances that support them.

@mjbvz Does that sound good to you?

[arcanis]

This is temporary and will be replaced by the strategy described in the description, if greenlighted.

[mjbvz]

This proposed solution is a step in the right direction.

As you noted, the failure mode of this change is that the entire TS server crashes (and such crashes would result in difficult to investigate issues being opened against VS Code). With your proposed change, how is the zip support actually implemented? A server plugin? A patched version of the server?

In either case, we need to be able to guarantee that the server can handle the file before we send it over. typescript.schemes may be enough but I need to know more about the implementation to say for sure

[arcanis]

There are two key components:

• We've published a ZipFS VSCode extension that adds support to VSCode for the zip: scheme through the FileSystemProvider API. Sources are here.

• We've also published a tool (yarn dlx @yarnpkg/pnpify --sdk) that generates the necessary TS wrapper and configures VSCode to use it (tsdk + enablePromptUseWorkspaceTsdk). Without it, TS doesn't know what to do with zip: entries. The code for the wrapper can be found here.

I think my proposal would work, because:

• People accessing zip instances with the extension + the SDK would get the regular TS experience thanks to the SDK generator which would have automatically set the typescript.tsdk and typescript.schemes properties.

• People without the extension but with the SDK would never send the zip: paths to TS, and even if they could it would work (because then they'd be in the case described in the previous point).

• People with only the extension (but not the SDK) wouldn't have either of the typescript.tsdk and typescript.schemes settings, so the language server would refuse to enable TS on zip files per the rules established in my first post - replicating the current safe behaviour.

• People with the extension and the SDK but using the stock TS (rather than the local one) would see the language server refuse to enable TS on zip files, since the typescript.schemes setting would only apply when the local TS (referenced in typescript.tsdk) is in use.

The only problematic case would be people with the extension, without the SDK, still using a custom typescript.tsdk path. In this situation VSCode would send the zip path (because instructed to do so in the settings), which would cause the server to crash. I'm not sure this is really a problem given that the user will have made a conscious invalid configuration choice, similar as what would happen if they were setting typescript.tsdk to a bogus file.

[arcanis]

Ping? 😊

[mjbvz]

Thanks for the details. Now I understand that you are proxying tsserver.js itself rather than shipping a ts server plugin.

Have you talked with the TypeScript team about a better solution that could potentially be easily leveraged by other editors as well?

Also if a user runs Select Typescript version, they can can easily overwrite the typescript.tsdk that your scripts writes with a different local typescript install path. With the current proposal, I believe that we would then incorrectly enable zip paths and potentially crash the TS Server. How would this case be handled?

[arcanis]

Have you talked with the TypeScript team about a better solution that could potentially be easily leveraged by other editors as well?

I already have one critical PR opened there, the circumstances make it difficult for me to justify maintaining another one at the moment until this one gets solved first 😕

With the current proposal, I believe that we would then incorrectly enable zip paths and potentially crash the TS Server. How would this case be handled?

This would enter into the "[...] and if the local SDK is enabled" case I mentioned in the OP. Basically, if the user selects a different TS than the one pointed by the tsdk setting, isSupportedScheme will return the same protocols as if schemes didn't exist.

[mjbvz]

@arcanis I suggest following up more with the TS Team. It would be very nice if you did not have to patch tsserver like this

The case I'm concerned about:

"typescript.schemes": ["zip:"],
"typescript.tsdk": "node_modules/typescript/lib" // this is a local tsserver that does not understand zip paths

With the proposal I don't currently understand how we'd enforce that typescript.schemes is only enabled for a specific local typescript.tsdk path

[arcanis]

With the proposal I don't currently understand how we'd enforce that typescript.schemes is only enabled for a specific local typescript.tsdk path

In this situation we wouldn't be able to enforce it, although that would be the result of a specific user configuration, which I think is in a large part the user's responsibility. In a sense, I feel like this isn't unlike what would happen if the user was to point tsdk to a non-existent path, if you see what I mean.

[mjbvz]

That case is easy to trigger using the Select TypeScript version command (which will suggest the local version from node_modules) so I think we need to address it. If the tsdk does not exist, we can fall back to the builtin ts version. Here however, TS server would just repeatedly crash

My preference would be that the TypeScript server itself tells us which types of files it supports. If that will not work, then we need a contribution point that says: for this specific tsdk path, enable this set of file schemes.

To be honest this whole proposal feels weird because, as far as I know, you would be the only consumer of any new extension point. VS Code would get bugs reports related to it and have to maintain but we would not be testing or dogfooding it.

This sounds like a good opportunity to make TS work better with yarn2 out of the box (or at least add proper extension points so that it can do so)

[arcanis]

This sounds like a good opportunity to make TS work better with yarn2 out of the box (or at least add proper extension points so that it can do so)

Trust me, if I believed I could do it, I'd be already working on it! I'm not afraid of contributing to codebases that aren't mine. Unfortunately, my understanding of the situation is that the TS team doesn't plan on implementing extension points until they're sure they would satisfy every possible use case, and in practical terms I'm afraid that means "probably never".

To be honest this whole proposal feels weird because, as far as I know, you would be the only consumer of any new extension point. VS Code would get bugs reports related to it and have to maintain but we would not be testing or dogfooding it.

This is only true on some degree. After all, #59650 got created ~2 years ago by people with no relation to Yarn, and it's a significant problem for the virtual filesystem API in general. Perhaps there isn't anyone else that would benefit from this work at the moment simply because noone else could extract enough value from virtual filesystems without TS support?

That case is easy to trigger using the Select TypeScript version command (which will suggest the local version from node_modules) so I think we need to address it. (...) We need a contribution point that says: for this specific tsdk path, enable this set of file schemes.

I see - in my case there is no node_modules, so I didn't consider that VSCode would propose it as well - but that's not a problem, is it? It's just a matter of checking that the currently selected VSCode module path matches the one configured in the tsdk setting. Something like this, in pseudo-code:

// Instead of
return currentActiveTsdk !== builtin.tsdk ? defaultSchemes + customSchemes : defaultSchemes;

// Prefer
return currentActiveTsdk === typescript.tsdk ? defaultSchemes + customSchemes : defaultSchemes;

[mjbvz]

Yes we need someone to drive a discussion with TypeScript about what the proper solution would be for #59650. Maybe TS needs to delegate certain file reads back to clients like VS Code for example. Patching the tsserver though is not scalable

Can you link to where this has been discussed with TS previously? Would microsoft/TypeScript#35206 cover your use case?

[arcanis]

Can you link to where this has been discussed with TS previously?

This comment (also kinda echoed by microsoft/TypeScript#28289 (comment)) expressed the intent of finding a general purpose solution (it was also mentioned in a prior meeting I had with the TS team).

Plugins are discussed on #16607 and #38736; they were still investigated in the 3.5, 3.6, 3.7, 3.8 roadmaps, but disappeared from the 3.9. I imagine this was intentional and they decided it would be too much work, although I don't have details.

Would microsoft/TypeScript#35206 cover your use case?

Not entirely, as it only covers native PnP resolution, not the part where TypeScript is able to load files from different protocols (it's not a problem when going through our SDK because it works transparently, but for a no-proxy approach TS would have to implement both resolution and filesystem hooks).

[mjbvz]

I talked with the TypeScript team this morning about this. We think that this needs more thought.

We are concerned that the current approach will spawn bugs that are difficult to triage (such as microsoft/TypeScript#39331 which took one of our engineers half a day to track down). The current yarn approach to proxying and patching tsserver seems like it is going to cause problems down the road, both for users and for us as maintainers

More broadly though, we'd like to see how all the current proposals around modules evolve. The root cause of these issue is that yarn 2 introduced a non-standardized solution to modules. Hopefully whatever is standardized can also be applied to or adopted by yarn. That would be the ideal outcome.

As a last resort, TypeScript would be open to adding functionality to enable reading files from VS Code file system providers. This would benefit other virtual file system providers as well. Again though, this would only be a last resort and definitely make sure to talk with the TS team before looking into this.

Sorry to have to say no to what I know seems like a very small change.

[arcanis]

As you can imagine, we feel very disappointed and let down, which is never a good feeling - even less when you believe you're working for the embetterment of an ecosystem. I know this isn't on you, and I appreciate receiving a straight answer.