From ffea7c3b85aa5425de49b3b7405bf28175007c16 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Thu, 28 Jul 2022 15:42:36 -0400 Subject: [PATCH 1/4] fix: handle all type-only imports by piping TS imports - `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 --- __tests__/integration/errors.spec.ts | 24 ++++---- __tests__/integration/fixtures/no-errors.ts | 11 ++++ .../integration/fixtures/no-errors/index.ts | 2 +- .../no-errors/type-only-import-import.ts | 1 + .../fixtures/no-errors/type-only-import.ts | 3 + __tests__/integration/helpers.ts | 7 ++- __tests__/integration/no-errors.spec.ts | 56 +++++++++++-------- __tests__/integration/watch.spec.ts | 10 +--- src/index.ts | 23 +++++++- 9 files changed, 91 insertions(+), 46 deletions(-) create mode 100644 __tests__/integration/fixtures/no-errors.ts create mode 100644 __tests__/integration/fixtures/no-errors/type-only-import-import.ts diff --git a/__tests__/integration/errors.spec.ts b/__tests__/integration/errors.spec.ts index e6cb2aa8..3ecacdec 100644 --- a/__tests__/integration/errors.spec.ts +++ b/__tests__/integration/errors.spec.ts @@ -5,7 +5,7 @@ import { normalizePath as normalize } from "@rollup/pluginutils"; import * as fs from "fs-extra"; import { RPT2Options } from "../../src/index"; -import * as helpers from "./helpers"; +import { findName, genBundle as genBundleH } from "./helpers"; // increase timeout to 15s for whole file since CI occassionally timed out -- these are integration and cache tests, so longer timeout is warranted jest.setTimeout(15000); @@ -21,7 +21,7 @@ afterAll(async () => { async function genBundle(relInput: string, extraOpts?: RPT2Options, onwarn?: Mock) { const input = local(`fixtures/errors/${relInput}`); - return helpers.genBundle({ + return genBundleH({ input, tsconfig: local("fixtures/errors/tsconfig.json"), testDir, @@ -41,10 +41,11 @@ test("integration - semantic error - abortOnError: false / check: false", async const { output: output2 } = await genBundle("semantic.ts", { check: false }, onwarn); expect(output).toEqual(output2); - expect(output[0].fileName).toEqual("index.js"); - expect(output[1].fileName).toEqual("semantic.d.ts"); - expect(output[2].fileName).toEqual("semantic.d.ts.map"); - expect(output.length).toEqual(3); // no other files + const files = ["index.js", "semantic.d.ts", "semantic.d.ts.map"]; + files.forEach(file => { + expect(findName(output, file)).toBeTruthy(); + }); + expect(output.length).toEqual(files.length); // no other files expect(onwarn).toBeCalledTimes(1); }); @@ -80,11 +81,10 @@ test("integration - type-only import error - abortOnError: false / check: false" }, onwarn); expect(output).toEqual(output2); - expect(output[0].fileName).toEqual("index.js"); - expect(output[1].fileName).toEqual("import-type-error.d.ts"); - expect(output[2].fileName).toEqual("import-type-error.d.ts.map"); - expect(output[3].fileName).toEqual("type-only-import-with-error.d.ts"); - expect(output[4].fileName).toEqual("type-only-import-with-error.d.ts.map"); - expect(output.length).toEqual(5); // no other files + const files = ["index.js", "import-type-error.d.ts", "import-type-error.d.ts.map", "type-only-import-with-error.d.ts.map", "type-only-import-with-error.d.ts.map"]; + files.forEach(file => { + expect(findName(output, file)).toBeTruthy(); + }); + expect(output.length).toEqual(files.length); // no other files expect(onwarn).toBeCalledTimes(1); }); diff --git a/__tests__/integration/fixtures/no-errors.ts b/__tests__/integration/fixtures/no-errors.ts new file mode 100644 index 00000000..f3cf653d --- /dev/null +++ b/__tests__/integration/fixtures/no-errors.ts @@ -0,0 +1,11 @@ +export const filesArr = [ + "index.js", + "index.d.ts", + "index.d.ts.map", + "some-import.d.ts", + "some-import.d.ts.map", + "type-only-import.d.ts", + "type-only-import.d.ts.map", + "type-only-import-import.d.ts", + "type-only-import-import.d.ts.map", +]; diff --git a/__tests__/integration/fixtures/no-errors/index.ts b/__tests__/integration/fixtures/no-errors/index.ts index 9ef3266a..c5668f5a 100644 --- a/__tests__/integration/fixtures/no-errors/index.ts +++ b/__tests__/integration/fixtures/no-errors/index.ts @@ -6,6 +6,6 @@ import { difference } from "./some-import"; export const diff2 = difference; // add an alias so that this file has to change when the import does (to help test cache invalidation etc) export { difference } from "./some-import" -export type { num } from "./type-only-import" +export type { num, num2 } from "./type-only-import" export { identity } from "./some-js-import" diff --git a/__tests__/integration/fixtures/no-errors/type-only-import-import.ts b/__tests__/integration/fixtures/no-errors/type-only-import-import.ts new file mode 100644 index 00000000..beb9256e --- /dev/null +++ b/__tests__/integration/fixtures/no-errors/type-only-import-import.ts @@ -0,0 +1 @@ +export type numb = number; diff --git a/__tests__/integration/fixtures/no-errors/type-only-import.ts b/__tests__/integration/fixtures/no-errors/type-only-import.ts index 5779bdf7..7066a6f7 100644 --- a/__tests__/integration/fixtures/no-errors/type-only-import.ts +++ b/__tests__/integration/fixtures/no-errors/type-only-import.ts @@ -1 +1,4 @@ +import type { numb } from "./type-only-import-import"; + export type num = number; +export type num2 = numb; diff --git a/__tests__/integration/helpers.ts b/__tests__/integration/helpers.ts index 99b3c19f..ecd03ad2 100644 --- a/__tests__/integration/helpers.ts +++ b/__tests__/integration/helpers.ts @@ -1,4 +1,4 @@ -import { rollup, watch, RollupOptions, OutputOptions, OutputAsset, RollupWatcher } from "rollup"; +import { rollup, watch, RollupOptions, OutputOptions, RollupOutput, OutputAsset, RollupWatcher } from "rollup"; import * as path from "path"; import rpt2, { RPT2Options } from "../../src/index"; @@ -72,3 +72,8 @@ export async function watchBundle (inputArgs: Params) { await watchEnd(watcher); // wait for build to end before returning, similar to genBundle return watcher; } + +export function findName (output: RollupOutput['output'], name: string) { + // type-cast to simplify type-checking -- [0] is always chunk, rest are always asset in our case + return output.find(file => file.fileName === name) as OutputAsset; +} diff --git a/__tests__/integration/no-errors.spec.ts b/__tests__/integration/no-errors.spec.ts index 941a26fb..dce9c671 100644 --- a/__tests__/integration/no-errors.spec.ts +++ b/__tests__/integration/no-errors.spec.ts @@ -1,11 +1,11 @@ import { jest, afterAll, test, expect } from "@jest/globals"; import * as path from "path"; import * as fs from "fs-extra"; -import { OutputAsset } from "rollup"; import { normalizePath as normalize } from "@rollup/pluginutils"; import { RPT2Options } from "../../src/index"; -import * as helpers from "./helpers"; +import { filesArr } from "./fixtures/no-errors"; +import { findName, genBundle as genBundleH } from "./helpers"; // increase timeout to 15s for whole file since CI occassionally timed out -- these are integration and cache tests, so longer timeout is warranted jest.setTimeout(15000); @@ -17,7 +17,7 @@ const fixtureDir = local("fixtures/no-errors"); afterAll(() => fs.remove(testDir)); async function genBundle(relInput: string, extraOpts?: RPT2Options) { - return helpers.genBundle({ + return genBundleH({ input: `${fixtureDir}/${relInput}`, tsconfig: `${fixtureDir}/tsconfig.json`, testDir, @@ -33,24 +33,33 @@ test("integration - no errors", async () => { const { output: outputWithCache } = await genBundle("index.ts"); expect(output).toEqual(outputWithCache); - expect(output[0].fileName).toEqual("index.js"); - expect(output[1].fileName).toEqual("index.d.ts"); - expect(output[2].fileName).toEqual("index.d.ts.map"); - expect(output[3].fileName).toEqual("some-import.d.ts"); - expect(output[4].fileName).toEqual("some-import.d.ts.map"); - expect(output[5].fileName).toEqual("type-only-import.d.ts"); - expect(output[6].fileName).toEqual("type-only-import.d.ts.map"); - expect(output.length).toEqual(7); // no other files + const files = filesArr; + files.forEach(file => { + expect(findName(output, file)).toBeTruthy(); + }); + expect(output.length).toEqual(files.length); // no other files // JS file should be bundled by Rollup, even though rpt2 does not resolve it (as Rollup natively understands ESM) expect(output[0].code).toEqual(expect.stringContaining("identity")); // declaration map sources should be correctly remapped (and not point to placeholder dir, c.f. https://github.com/ezolenko/rollup-plugin-typescript2/pull/221) - const decMapSources = JSON.parse((output[2] as OutputAsset).source as string).sources; + const decMap = findName(output, "index.d.ts.map"); + const decMapSources = JSON.parse(decMap.source as string).sources; const decRelPath = normalize(path.relative(`${testDir}/dist`, `${fixtureDir}/index.ts`)); expect(decMapSources).toEqual([decRelPath]); }); +test("integration - no errors - using files list", async () => { + const { output } = await genBundle("index.ts", { tsconfigOverride: { files: ["index.ts"] } }); + + // should still have the type-only import and type-only import import! + const files = filesArr; + files.forEach(file => { + expect(findName(output, file)).toBeTruthy(); + }); + expect(output.length).toEqual(files.length); // no other files +}); + test("integration - no errors - no declaration maps", async () => { const noDeclarationMaps = { compilerOptions: { declarationMap: false } }; const { output } = await genBundle("index.ts", { @@ -58,11 +67,11 @@ test("integration - no errors - no declaration maps", async () => { clean: true, }); - expect(output[0].fileName).toEqual("index.js"); - expect(output[1].fileName).toEqual("index.d.ts"); - expect(output[2].fileName).toEqual("some-import.d.ts"); - expect(output[3].fileName).toEqual("type-only-import.d.ts"); - expect(output.length).toEqual(4); // no other files + const files = filesArr.filter(file => !file.endsWith(".d.ts.map")); + files.forEach(file => { + expect(findName(output, file)).toBeTruthy(); + }); + expect(output.length).toEqual(files.length); // no other files }); @@ -88,12 +97,15 @@ test("integration - no errors - allowJs + emitDeclarationOnly", async () => { }, }); - expect(output[0].fileName).toEqual("index.js"); - expect(output[1].fileName).toEqual("some-js-import.d.ts"); - expect(output[2].fileName).toEqual("some-js-import.d.ts.map"); - expect(output.length).toEqual(3); // no other files + const files = ["index.js", "some-js-import.d.ts", "some-js-import.d.ts.map"]; + files.forEach(file => { + expect(findName(output, file)).toBeTruthy(); + }); + expect(output.length).toEqual(files.length); // no other files expect(output[0].code).toEqual(expect.stringContaining("identity")); expect(output[0].code).not.toEqual(expect.stringContaining("sum")); // no TS files included - expect("source" in output[1] && output[1].source).toEqual(expect.stringContaining("identity")); + + const dec = findName(output, "some-js-import.d.ts"); + expect(dec.source).toEqual(expect.stringContaining("identity")); }); diff --git a/__tests__/integration/watch.spec.ts b/__tests__/integration/watch.spec.ts index ce8ced6d..2c329c06 100644 --- a/__tests__/integration/watch.spec.ts +++ b/__tests__/integration/watch.spec.ts @@ -3,6 +3,7 @@ import * as path from "path"; import * as fs from "fs-extra"; import { RPT2Options } from "../../src/index"; +import { filesArr } from "./fixtures/no-errors"; import * as helpers from "./helpers"; // increase timeout to 15s for whole file since CI occassionally timed out -- these are integration and cache tests, so longer timeout is warranted @@ -36,15 +37,6 @@ test("integration - watch", async () => { const distPath = `${testDir}/dist/index.js`; const decPath = `${distDir}/index.d.ts`; const decMapPath = `${decPath}.map`; - const filesArr = [ - "index.js", - "index.d.ts", - "index.d.ts.map", - "some-import.d.ts", - "some-import.d.ts.map", - "type-only-import.d.ts", - "type-only-import.d.ts.map", - ]; const watcher = await watchBundle(srcPath); diff --git a/src/index.ts b/src/index.ts index 2ac5c106..ad29e793 100644 --- a/src/index.ts +++ b/src/index.ts @@ -33,6 +33,7 @@ const typescript: PluginImpl = (options) => let documentRegistry: tsTypes.DocumentRegistry; // keep the same DocumentRegistry between watch cycles let cache: TsCache; let noErrors = true; + let transformedFiles: Set; const declarations: { [name: string]: { type: tsTypes.OutputFile; map?: tsTypes.OutputFile } } = {}; const checkedFiles = new Set(); @@ -150,6 +151,9 @@ const typescript: PluginImpl = (options) => cache = new TsCache(pluginOptions.clean, pluginOptions.objectHashIgnoreUnknownHack, servicesHost, pluginOptions.cacheRoot, parsedConfig.options, rollupOptions, parsedConfig.fileNames, context); + // reset transformedFiles Set on each watch cycle + transformedFiles = new Set(); + // printing compiler option errors if (pluginOptions.check) { const diagnostics = convertDiagnostic("options", service.getCompilerOptionsDiagnostics()); @@ -203,8 +207,10 @@ const typescript: PluginImpl = (options) => return null; }, - transform(code, id) + async transform(code, id) { + transformedFiles.add(id); + if (!filter(id)) return undefined; @@ -234,6 +240,21 @@ const typescript: PluginImpl = (options) => if (!result) return undefined; + // handle all type-only imports by resolving + loading all of TS's references + // Rollup can't see these otherwise, because they are "emit-less" and produce no JS + if (result.references) { + let modules = await Promise.all(result.references + .filter(ref => !ref.endsWith(".d.ts")) + .map(ref => this.resolve(ref, id))); + + // wait for all to be loaded (otherwise, as this is async, some may end up only loading after `generateBundle`) + await Promise.all(modules.map(async module => { + if (!module || transformedFiles.has(module.id)) // check for circular references (per https://rollupjs.org/guide/en/#thisload) + return; + await this.load({id: module.id}); + })); + } + if (watchMode && result.references) { if (tsConfigPath) From 064aee26cb9e898aebc870f194962fb0cbe76c99 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Fri, 29 Jul 2022 14:15:14 -0400 Subject: [PATCH 2/4] move type-only recursion to after declarations are added to the `declarations` dict - preserve some ordering and simplify future debugging - also fix lint issue, `let modules` -> `const modules` - I previously changed it (while WIP), but now it's static/never reassigned, so can use `const` --- src/index.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/index.ts b/src/index.ts index ad29e793..e41ec333 100644 --- a/src/index.ts +++ b/src/index.ts @@ -240,10 +240,21 @@ const typescript: PluginImpl = (options) => if (!result) return undefined; + if (watchMode && result.references) + { + if (tsConfigPath) + this.addWatchFile(tsConfigPath); + + result.references.map(this.addWatchFile, this); + context.debug(() => `${green(" watching")}: ${result.references!.join("\nrpt2: ")}`); + } + + addDeclaration(id, result); + // handle all type-only imports by resolving + loading all of TS's references // Rollup can't see these otherwise, because they are "emit-less" and produce no JS if (result.references) { - let modules = await Promise.all(result.references + const modules = await Promise.all(result.references .filter(ref => !ref.endsWith(".d.ts")) .map(ref => this.resolve(ref, id))); @@ -255,17 +266,6 @@ const typescript: PluginImpl = (options) => })); } - if (watchMode && result.references) - { - if (tsConfigPath) - this.addWatchFile(tsConfigPath); - - result.references.map(this.addWatchFile, this); - context.debug(() => `${green(" watching")}: ${result.references!.join("\nrpt2: ")}`); - } - - addDeclaration(id, result); - // if a user sets this compilerOption, they probably want another plugin (e.g. Babel, ESBuild) to transform their TS instead, while rpt2 just type-checks and/or outputs declarations // note that result.code is non-existent if emitDeclarationOnly per https://github.com/ezolenko/rollup-plugin-typescript2/issues/268 if (parsedConfig.options.emitDeclarationOnly) From b3936787d84c8430fdc4b5a3d3465dcf27bc5f5d Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Fri, 29 Jul 2022 14:47:43 -0400 Subject: [PATCH 3/4] rewrite the core logic with a `for...of` loop instead - simpler to follow than `map` + `filter` + `Promise.all` - might(?) be faster without `Promise.all` as well as more can happen async without waiting - (I'm not totally sure of the low-level implementation of async to know for sure though) --- src/index.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index e41ec333..28e629d9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -254,16 +254,17 @@ const typescript: PluginImpl = (options) => // handle all type-only imports by resolving + loading all of TS's references // Rollup can't see these otherwise, because they are "emit-less" and produce no JS if (result.references) { - const modules = await Promise.all(result.references - .filter(ref => !ref.endsWith(".d.ts")) - .map(ref => this.resolve(ref, id))); + for (const ref of result.references) { + if (ref.endsWith(".d.ts")) + continue; - // wait for all to be loaded (otherwise, as this is async, some may end up only loading after `generateBundle`) - await Promise.all(modules.map(async module => { + const module = await this.resolve(ref, id); if (!module || transformedFiles.has(module.id)) // check for circular references (per https://rollupjs.org/guide/en/#thisload) - return; + continue; + + // wait for all to be loaded (otherwise, as this is async, some may end up only loading after `generateBundle`) await this.load({id: module.id}); - })); + } } // if a user sets this compilerOption, they probably want another plugin (e.g. Babel, ESBuild) to transform their TS instead, while rpt2 just type-checks and/or outputs declarations From 4cde28935593f8a7782625e9a0c41821b95231b7 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sat, 30 Jul 2022 15:29:53 -0400 Subject: [PATCH 4/4] add comment about normalization --- src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.ts b/src/index.ts index 28e629d9..8b8d552b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -209,7 +209,7 @@ const typescript: PluginImpl = (options) => async transform(code, id) { - transformedFiles.add(id); + transformedFiles.add(id); // note: this does not need normalization as we only compare Rollup <-> Rollup, and not Rollup <-> TS if (!filter(id)) return undefined;