Skip to content

Commit

Permalink
fix: hardcode declaration extension check (#456)
Browse files Browse the repository at this point in the history
* fix: preemptively `filter` type-only files to workaround Rollup bug

- 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

* refactor: use a common function for resolution checks

- 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`

* feat: also ignore `.d.cts` and `.d.mts` file extensions

- 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

* fix comment -- there's more reasons why we shouldn't resolve all references

- 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
  • Loading branch information
agilgur5 committed Jul 10, 2023
1 parent 61c7392 commit 976dadb
Showing 1 changed file with 14 additions and 5 deletions.
19 changes: 14 additions & 5 deletions src/index.ts
Expand Up @@ -80,6 +80,17 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
context.debug(() => `${blue("generated declarations")} for '${key}'`);
}

/** common resolution check -- only resolve files that aren't declarations and pass `filter` */
const shouldResolve = (id: string): boolean => {
if (id.endsWith(".d.ts") || id.endsWith(".d.cts") || id.endsWith(".d.mts"))
return false;

if (!filter(id))
return false;

return true;
}

/** to be called at the end of Rollup's build phase, before output generation */
const buildDone = (): void =>
{
Expand Down Expand Up @@ -207,10 +218,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
if (!resolved)
return;

if (resolved.endsWith(".d.ts"))
return;

if (!filter(resolved))
if (!shouldResolve(resolved))
return;

cache.setDependency(resolved, importer);
Expand Down Expand Up @@ -277,7 +285,8 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
// Rollup can't see these otherwise, because they are "emit-less" and produce no JS
if (result.references && supportsThisLoad) {
for (const ref of result.references) {
if (!filter(ref))
// pre-emptively filter out files that we don't resolve ourselves (e.g. declarations). don't add new files to Rollup's pipeline if we can't resolve them
if (!shouldResolve(ref))
continue;

const module = await this.resolve(ref, id);
Expand Down

0 comments on commit 976dadb

Please sign in to comment.