fix: handle all type-only imports by piping TS imports

agilgur5 commented 2 years ago

~NOTE: this is built on top of #403 as it changes its code a bit. #403 is itself built on top of #386. As such, I've marked this PR as "Draft" until those PRs are merged.~ Rebased on top and marked as ready for review. (surprisingly clean rebase)

Also this might be the first time this repo has more open PRs than open issues 😮

Summary

Completely handle the long-standing issue of type-only imports

Uses the solution I proposed in my root cause analysis in https://github.com/ezolenko/rollup-plugin-typescript2/issues/298#issuecomment-1146658442
Fixes #7 as all type-only imports go through the resolveId hook now, which adds them to the cache (calls cache().setDependency)
Fixes #211
- And its various duplicates: fixes #24, fixes #106, fixes #409
Fully fixes the most common issue that I mentioned in https://github.com/ezolenko/rollup-plugin-typescript2/pull/311#issuecomment-1115491161, type-only imports
Related to #345
- That PR fixed #298 and the latter half of #254 for their specific use-cases as they happen to use include globs, but this PR fixes it in all other scenarios as well (namely when using files instead of include globs)

Details

result.references is populated by ts.preProcessFile; i.e. this is TS discovering all imports, instead of Rollup
- TS's imports include type-only files as TS understands those (whereas they aren't emitted in the JS for Rollup to see, since, well, they produce no JS)
- so we can pipe all these through Rollup's this.resolve and this.load to make them go through Rollup's resolveId -> load -> transform hooks
- this makes sure that other plugins on the chain get to resolve/transform them as well
- and it makes sure that we run the same code that we run on all other files on type-only ones too
  - for instance: adding declarations, type-checking, setting them as deps in the cache graph, etc
  - yay recursion!
  - also add check for circular references b/c of this recursion (which Rollup docs confirm is necessary, per in-line comment)
- and Rollup ensures that there is no perf penalty if a regular file is processed this way either, as it won't save the hook results when it appears in JS (i.e. Rollup's module graph)
  - we are checking more files though, so that in and of itself means potential slowdown for better correctness
add a test for this that uses a tsconfig files array, ensuring that the include workaround won't cover these type-only files
- this test fails without the new code added to index in this commit
- also add another file, type-only-import-import, to the no-errors fixture to ensure that we're not just checking imports one level deep, and actually going through type-only imports of type-only imports as well
- the declaration check for this will fail if type-only imports are not handled recursively
  - an initial version of this fix that I had that didn't call this.load failed this check
refactor(test): make the integration tests more resilient to output ordering changes
- due to the eager calls to this.load, the ordering of declaration and declaration map outputs in the bundle changed
- and bc TS's default ordering of imports seems to differ from Rollup's
- note that this only changed the order of the "bundle output" object -- which Rollup doesn't guarantee ordering of anyway
- all files are still in the bundle output and are still written to disk
- for example, the watch tests did not rely on this ordering and as such did not need to change due to the ordering change
- create a findName helper that will search the output array instead, ensuring that most ordering does not matter
- we do still rely on output[0] being the bundled JS (ESM) file, however
refactor(test): go through a files array for tests that check for multiple files instead of listing out each individual check
- this makes the tests more resilient to fixture changes as well (i.e. addition / deletion of files)
- create no-errors.ts that exports a list of files for this fixture
- didn't need to do the same for errors.ts as of yet; may do so in the future though

Review Notes

I double-checked that the transform hook could be async in the minimum Rollup version we support.
- The in-line linked docs are from Rollup v1.18.0 as I couldn't find a mention of async in the CHANGELOG.md.
This could be considered "breaking", but the bundle ordering should realistically not affect anyone (only tests that rely on ordering, which was never guaranteed by Rollup as this.load could be called by any plugin at any time) and this should only be additive, in that it increases the number of files that are type-checked / generated declarations for.
- So personally, I don't think this needs a minor bump. If it does get a minor bump, we should release a patch before merging this as I've had a bunch of other fixes in the past month or two that have not yet been released.
Related to that, I specifically did not touch the "missed" type-checking / declaration generation that uses parsedConfig.fileNames to partly workaround this issue (i.e. #345 and the respective declaration generation block)
- Removing those would almost certainly be considered breaking, since it could remove some files from type-checking / declaration generation. See also the "Note" in #345
- While #211 says rpt2 shouldn't process those, I don't necessarily agree with that statement, because tsc will process those. Not processing parsedConfig.fileNames would mean only processing the Rollup input, acting as if the tsconfig had files with only that single file from the Rollup input in it. That behavior could make sense to some, but not others, so I'm not sure that that's ideal. The "Note" in #345 mentions how users may use include globs or files arrays to list other files they want type-checking and declarations. These files may not be considered part of the Rollup "bundle" however, so I could see an argument for either direction.
- In any case, this has not been changed for now. If warranted, we could remove this in a separate PR, and that one would be breaking, but I would not say that this PR in its current state is breaking.

References

This also fixes various downstream issues:

and probably (hard to tell without more details from issue author):

agilgur5 commented 2 years ago

While #211 says rpt2 shouldn't process those, I don't necessarily agree with that statement, because tsc will process those. Not processing parsedConfig.fileNames would mean only processing the Rollup input, acting as if the tsconfig had files with only that single file from the Rollup input in it. That behavior could make sense to some, but not others, so I'm not sure that that's ideal. [...] These files may not be considered part of the Rollup "bundle" however, so I could see an argument for either direction.

I happened to stumble upon this yesterday -- it turns out that ts-loader (in Webpack-land) actually does this as well, acting like tsc by default, with the onlyCompileBundledFiles option available to do, as named, only consider files that are part of the "bundle".

So doing the same as ts-loader's default is probably optimal community-wise. I don't think we necessarily need the onlyCompileBundledFiles option though, as the same is achievable by changing tsconfig files (in a tsconfigOverride, for instance)

ezolenko commented 2 years ago

There is a conflict with previous PR, but otherwise looks good

agilgur5 commented 2 years ago

Fixed the conflict 👍

agilgur5 commented 1 year ago

Welp, realized while testing something that this.load was only added in Rollup 2.60.0, much later than this.resolve...

That's my bad, adding a hotfix now to make this backward-compatible with our minimum 1.26.3 requirement -- see #424

ezolenko / rollup-plugin-typescript2