-
Notifications
You must be signed in to change notification settings - Fork 1
[WIP] Implement yarn pnp support #1
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
base: main
Are you sure you want to change the base?
Conversation
13aa2a3 to
1e204b1
Compare
internal/module/resolver.go
Outdated
| func (r *resolutionState) loadModuleFromNearestNodeModulesDirectoryWorker(ext extensions, mode core.ResolutionMode, typesScopeOnly bool) *resolved { | ||
| pnpApi := pnp.GetPnpApi(r.containingDirectory) | ||
| if pnpApi != nil { | ||
| // TODO: stop at global cache too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At L927, they have a comment saying they should stop at global cache, but it looks like it's not implemented yet
So I added a comment for the pnp resolver too
| } | ||
|
|
||
| if pathObj.IsRedirect && !isPackageRootPath { | ||
| return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those empty strings also used by stock TS-go? Feels like it should at least be a constant declared somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's the exact same implementation as tryGetModuleNameAsNodeModule from L815 to L901, with minor changes to make pnp work (L803 is also defined at L886)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However this means I had to duplicate some code (see this comment), so I'm not sure if it should stay like this
40159a2 to
b681451
Compare
e17c10f to
ee62041
Compare
ee62041 to
c13f422
Compare
Motivation
This PR adds Plug'n'Play support natively to Typescript Go, following this issue:
It has been reviewed and supported by @arcanis, the lead maintainer of Yarn, and the original author of Yarn PnP.
Datadog has a frontend monorepo using yarn with over 6k packages, and seeing how TS Strada struggles with our current scaling, we decided to invest time in adding a native Yarn PnP support for Typescript Go.
This PnP implementation has been actively used in the IDE of more than 230 engineers at Datadog, and we're committed to fixing all issues reported to us.
Challenges
We did not integrate it in our CI yet as we still have several packages failing on build mode (most errors seem to be reported in the issues section of TS Corsa). Because the TS Corsa API is not available yet, we also couldn't integrate it properly with a fast lage setup unlike with the TS Strada API.
Changes
It's based on the main changes from the original yarn patch (microsoft/TypeScript@99f3e13) that the community has been maintaining for years throughout Typescript Strada updates, except that we implemented the official PnP specification so it doesn't depend on third-party code.
Implemented features:
internal/module/resolver.gointernal/modulespecifiers/specifiers.gointernal/core/compileroptions.goMissing features:
pnpApi.ResolveToUnqualified.pnp.cjschangesTests
internal/fourslash)