-
Couldn't load subscription status.
- Fork 70
fix: add realpath to host to properly resolve monorepos / symlinks
#332
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
Conversation
- tested this in a pnpm repo with symlinked deps and it worked there,
so I believe this fixes all pnpm issues
- it may also fix some Lerna issues if they were due to symlinks, but
I didn't check those
- not sure about others, e.g. Rush, Yarn workspaces, Yarn PnP
- I figured out this was needed by staring at the TS source code and
then I found this line:
https://github.com/microsoft/TypeScript/blob/67673f324dd5f9398bb53fd16bf75efd155c32e7/src/compiler/moduleNameResolver.ts#L1412
- it expects `host.realpath` to be implemented for TS's `realPath` to
work correctly, otherwise it just returns the path with no
transformation (i.e. the path to the symlink instead of the
realpath)
- this is not documented _anywhere_ and we were hitting this when
calling `getEmitOutput`, before even using `moduleNameResolver`
- so I just tried implementing it... and it worked!
- notably, the other Rollup TS plugins don't implement this either???
- not sure how they don't error on this??
- note that I added a `!` as `realpath` doesn't have to be implemented
on `ts.sys`... but it is in the default implementation (see comment)
- I originally had a ternary with `fs.realpathSync` if it didn't exist
but that is literally what the default implementation uses
- can add this back in the future if it becomes an issue
90caf5f to
72651ff
Compare
|
I fixed the tests I added for this on Windows (which I suspected might error there). Just need #331 to be merged so that this can be rebased on top, then everything should pass. CI is currently failing on Windows due to #329 (comment) being merged early and |
|
This one needs a release I guess |
yep, that it would. think I'll get a few more PRs in today for bugfixes if you want to batch a few things together instead |
|
Ok, let me know when you have everything you had in mind in. |
|
@ezolenko #334 and #338 were the last two fixes I was looking to get in, so ready for a batched release now 👍 Note that you'll need to do a (Anything else that I'm working on is going to take longer to figure out the right approach, fix, and test, but think I've made great progress on bug squashing so far!) |
|
Released in 0.32.0 🎉 Just so you know, I edited the release notes to add sections by order of importance. This is similar to how I organized releases in TSDX. Would like to automate this, but I've never quite gotten around to release automation in OSS and my changelog generator is outdated now. Also as a heads up, GitHub's auto-generated release notes seem to only include PRs, meaning it missed all the direct commits that you made. For the most part, those don't necessarily need call-outs in the release notes and can be seen in the full changelog (alternatively, we could also expand the "Internal" section and put a spoiler/ |
realpath to host to properly resolve monoreposrealpath to host to properly resolve monorepos / symlinks
Summary
Implement
realpathinhost.tsto properly resolve symlinks, i.e. for monoreposWhile the code for this is tiny, this is a pretty huge bugfix given the number of users that have run into this here and downstream (in
microbundle, TSDX, etc) -- glad to finally fix such a common error!Details
tested this in a
pnpmrepo with symlinked deps and it worked there, so I believe this fixes allpnpmissuespnpmas well, but Rush is compatible with multiple package managers, so unsure of others.I figured out this was needed by staring at the TS source code and then I found this line
host.realpathto be implemented for TS'srealPathto work correctly, otherwise it just returns the path with no transformation (i.e. the path to the symlink instead of the realpath)getEmitOutput, before even usingmoduleNameResolverSee my root cause analysis in #234 (comment) for (much) more details
Review Notes
!asrealpathdoesn't have to be implemented onts.sys... but it is in the default implementation (see comment)fs.realpathSyncif it didn't exist but that is literally what the default implementation usesReferences
pnpm? #330Possible fix for Vaguefailed to transpileerror --noEmitOnError: truebreaks rpt2 #254, which mentionspnpmsymlinks, but haven't testedfailed to transpileerror --noEmitOnError: truebreaks rpt2 #254 (comment) -- that's actually completely unrelated andpnpmand other things were totally red herrings to the root causeError: Could not find source file#214and Symlinks to files outside package root are not being compiled #188, which are Lerna repos, but haven't testedError: Could not find source file#214microbundleuses outdated version of rpt2 #295. Those were also monorepo issues, but they were fixed before this PR (incidentally, by upgrading@rollup/pluginutils, per the linked comments).d.tsfile with Vite and monorepo #274, which mentions "monorepo" and has a similar kind of issue with declaration output as Declaration is missing imported type when usingpnpm? #330, but there is no repro to test againstAlso possible fix for Monorepo example, using@rollup/plugin-alias#237, which has a workaround and mentions "monorepo", but unsure if related and no repro was provided there@rollup/plugin-alias#237 (comment) -- that actually seems to be correct behavior and the "workaround" is the proper solution. Unsure about the second, unrelated feature request/PR that was added but it solved itselfDownstream fixes: