fix: hardcode declaration extension check #456
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- basically, right now, the addition of `this.load` on all `references` is causing Rollup to error out on JSON files
- specifically, this is impacting `configPlugin` usage (i.e. `rollup.config.ts`), where previously one didn't need `@rollup/plugin-json`, but now it is erroring out without it
- I tracked this down to be because of `this.load` specifically
- to avoid this and similar such issues, we can preemptively `filter` out files before calling `this.resolve` / `this.load`, which should end up `exclude`ing JSON files and any other non-rpt2 files
- this should also make it a bit more efficient to skip some recursion
- and non-rpt2 files shouldn't include any type-only files
- confirmed that this change fixes the error
- and that the type-only tests still pass
- since the same logic is used in `resolveId` and these _should_ be equivalent
- in the future, we might want to add more common logic to this function, e.g. `getAllReferences` removes `undefined` and uses `moduleNameResolver` as well, similar to `resolveId`
- may not be so easy, so TBD
- for instance, even moving the `undefined` check into the func required adding a type guard, as the compiler wasn't quite able to infer that passing the func meant it was not `undefined`
- support newer TS extensions
- rpt2 should _always_ ignore declarations
- regardless of the `exclude`; as in, if a user accidentally removes declarations in an override, rpt2 should still not directly read declarations
- as they are normally read ambiently by TS and not _directly_ by Rollup or TS
…rences
- we don't `return false` in `resolveId`, so any new file that wasn't previously in Rollup's pipeline _must_ be resolved
- `return` just defers to the next plugin, so, for a declaration, it eventually causes Rollup to try and fail to resolve on its own, giving an `Unexptected token` error message
- but we _don't_ want to `return false` in `resolveId` if they _intentionally_ imported a declaration for some reason (e.g. if they're going to transform it in some way)
- if we did `return false`, no other plugin could process either
- so as a result, we should just never call `this.resolve()` on anything we don't expect to be able to resolve
- i.e. don't add anything new to the pipeline that we don't resolve ourselves
agilgur5
added
the
kind: regression
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #451 (comment), ensure declarations are always ignored during resolution.
Details
rpt2 should always ignore declarations
exclude; as in, if a user accidentally removes declarations in an override, rpt2 should still not directly read declarationsthis also ensures that the
resolveIdcheck is the same as the type-only resolve pre-checkCherry-pick of two commits from October that I had on a different, incomplete branch for #426