Closed agilgur5 closed 2 years ago
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 https://github.com/ezolenko/rollup-plugin-typescript2/pull/329#issuecomment-1138876847 being merged early and master
being currently broken as a result, not because of this PR.
This one needs a release I guess
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 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!)
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
https://github.com/ezolenko/rollup-plugin-typescript2/issues/216#issuecomment-1140024487) -- may be better to include in the release notes just in case.
Summary
Implement
realpath
inhost.ts
to 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
pnpm
repo with symlinked deps and it worked there, so I believe this fixes allpnpm
issuespnpm
as 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.realpath
to be implemented for TS'srealPath
to 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 usingmoduleNameResolver
See my root cause analysis in https://github.com/ezolenko/rollup-plugin-typescript2/issues/234#issuecomment-1139202740 for (much) more details
Review Notes
!
asrealpath
doesn't have to be implemented onts.sys
... but it is in the default implementation (see comment)fs.realpathSync
if it didn't exist but that is literally what the default implementation usesReferences
pnpm
symlinks, but haven't tested~pnpm
and other things were totally red herrings to the root cause@rollup/pluginutils
, per the linked comments)Downstream fixes: