Skip to content
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

fix: add realpath to host to properly resolve monorepos / symlinks #332

Merged
merged 2 commits into from
May 30, 2022

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented May 27, 2022

Summary

Implement realpath in host.ts to properly resolve symlinks, i.e. for monorepos

While 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 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
      • EDIT: yes, this fixed some Lerna issues too. Including Lerna + NPM
    • not sure about others, e.g. Rush, Yarn workspaces, Yarn PnP
      • EDIT: this fixed some Rush repos that used pnpm as well, but Rush is compatible with multiple package managers, so unsure of others.
    • EDIT: given the confirmed fixes in the edits above, this has likely fixed all symlink issues with all package managers
  • I figured out this was needed by staring at the TS source code and then I found this line

    • 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??

See my root cause analysis in #234 (comment) for (much) more details

Review Notes

  • 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

References

Downstream fixes:

- 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
@agilgur5 agilgur5 added kind: bug Something isn't working properly topic: monorepo / symlinks Related to monorepos and/or symlinks (Lerna, Yarn, PNPM, Rush, etc) labels May 27, 2022
@agilgur5
Copy link
Collaborator Author

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 master being currently broken as a result, not because of this PR.

@ezolenko
Copy link
Owner

This one needs a release I guess

@agilgur5
Copy link
Collaborator Author

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

@ezolenko
Copy link
Owner

ezolenko commented Jun 1, 2022

Ok, let me know when you have everything you had in mind in.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jun 1, 2022

@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 build && build-self beforehand as I didn't do that in my PRs (I normally leave dist/ for the release process to handle)

(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!)

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jun 2, 2022

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/<details> tag around it), but some, like the dependency updates -- which occasionally cause breaking changes (like in the case of @rollup/pluginutils's createFilter #216 (comment)) -- may be better to include in the release notes just in case.

@agilgur5 agilgur5 deleted the fix-monorepos-realpath branch July 2, 2023 21:26
Repository owner locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind: bug Something isn't working properly topic: monorepo / symlinks Related to monorepos and/or symlinks (Lerna, Yarn, PNPM, Rush, etc) topic: TS Compiler API Docs Related to the severely lacking TS Compiler API Docs
Projects
None yet
2 participants