Add pnp resolution support #1875
gun-yu
started this conversation in
Design Discussion
Replies: 1 comment 1 reply
-
I think a PR would be good. We wouldn't want to have the external dep, no. #460 would be the issue that is closed by it. |
Beta Was this translation helpful? Give feedback.
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Hello, I’m operating a large-scale JavaScript-based monorepo that uses Yarn PnP. As the repository has grown, the CI speed has started to negatively impact our team’s productivity. I wanted to adopt TypeScript Go to improve this, but after realizing it doesn’t support PnP resolution, I decided to contribute and open a PR to add it.
I’ve built a proof of concept that works well in our large repository,
👉 https://github.com/gun-yu/typescript-go/pull/1/files
but since merging it right away might not be appropriate, I’d like to request a design review from the TypeScript Go team.
Here are the design topics I’d like to discuss:
PnP-Go internalization
This is a Go implementation of Yarn PnP, referencing the official Yarn PnP implementation. All tests pass.
👉 https://github.com/gun-yu/pnp-go
Since the TypeScript Go team seems to prefer having controllable source code, I can internalize the source and test code into the repository if needed.
Initialization and placement of pnpResolutionConfig
For now, I plan to keep it in the host and inject it into the resolver.
In a monorepo setup, Yarn typically has only one pnp.cjs file, so I believe it’s fine to locate it once and reuse it.
If you have suggestions on the placement or injection method of pnpResolutionConfig, I’d be happy to discuss them.
Also, since there seem to be multiple hosts, I haven’t added injection in all places yet.
I suspect some are related to LSP behavior — if necessary, I can add that later or include it in the PR.
PnP VFS
This is a PnP VFS implementation for zip files.
I used @evanw’s implementation (https://github.com/evanw/esbuild/blob/main/internal/fs/fs_zip.go).
Thank you Evan for the great code!
It’s initialized in NewCompilerHost only when PnP is detected.
If there are no objections to this initialization point, I’ll leave it as is.
Using internal os and filePath functions
If using VFS consistently is required, I can internalize os and filepath usage in my code.
Otherwise, I’d prefer to keep the current implementation.
loadModuleFromSpecificNodeModulesDirectory interface change
I modified the interface so that PnP can also be used.
The interface might not be the easiest to read, but since it’s an internal function, I think it’s acceptable.
Because it now handles not just NodeModuleDirectory but also PnP paths, renaming the function might be appropriate.
I’d like to hear your thoughts on this.
Possible bug?
When passing through packageJsonInfoCache.Set, the trailing slash in packageDirectory is removed.
I’m not sure if this is intentional or a bug, so I’d like to get the TypeScript Go team’s opinion.
I’ve also added a comment about this in resolver_pnp.go.
If it’s fine to open a PR right away, I’m happy to discuss these points directly there as well.
Beta Was this translation helpful? Give feedback.
All reactions