From 2716344d35f8252d8653357c0ca436df0e88ac6e Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Sat, 4 Jun 2022 20:59:24 -0400 Subject: [PATCH 1/8] fix: type-check `include`d files missed by transform (type-only files) - type-only files never get processed by Rollup as their imports are elided/removed by TS in the resulting compiled JS file - so, as a workaround, make sure that all files in the `tsconfig` `include` (i.e. in `parsedConfig.fileNames`) are also type-checked - note that this will not catch _all_ type-only imports, in particular if one is using `tsconfig` `files` (or in general _not_ using glob patterns in `include`) -- this is just a workaround, that requires a separate fix - we do the same process for generating declarations for "missed" files right now in `_onwrite`, so basically do the same thing but for type-checking in `_ongenerate` (_technically_ speaking, there could be full TS files (i.e. _not_ type-only) that are in the `include` and weren't transformed - these would basically be independent TS files not part of the bundle that the user wanted type-checking and declarations for (as we _already_ generate declarations for those files)) --- src/index.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/index.ts b/src/index.ts index 0977063b..44cff2a9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -287,6 +287,23 @@ const typescript: PluginImpl = (options) => }); } + // type-check missed files as well + parsedConfig.fileNames.forEach((name) => + { + if (!pluginOptions.check) + return; + + const key = normalize(name); + if (key in declarations || !filter(key)) // don't duplicate if it's already been transformed + return; + + context.debug(() => `type-checking missed '${key}'`); + + const snapshot = servicesHost.getScriptSnapshot(key); + if (snapshot) + typecheckFile(key, snapshot, context); + }); + if (!watchMode && !noErrors) context.info(yellow("there were errors or warnings.")); From 53604ff8302ce2fe492b7c4f26d0e88f70b095bd Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Tue, 28 Jun 2022 17:56:39 -0400 Subject: [PATCH 2/8] move misssed type-checking to `buildEnd` hook, remove declarations check - `buildEnd` is a more correct place for it, since this does not generate any output files - (unlike the missed declarations) - and this means it's only called once per build, vs. once per output - remove the check against the `declarations` dict as one can type-check without outputting declarations - i.e. if `declaration: false`; not the most common use-case for rpt2, but it is one --- src/index.ts | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/index.ts b/src/index.ts index 44cff2a9..042f632f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -253,15 +253,32 @@ const typescript: PluginImpl = (options) => buildEnd(err) { - if (!err) + if (err) + { + // workaround: err.stack contains err.message and Rollup prints both, causing duplication, so split out the stack itself if it exists (c.f. https://github.com/ezolenko/rollup-plugin-typescript2/issues/103#issuecomment-1172820658) + const stackOnly = err.stack?.split(err.message)[1]; + if (stackOnly) + this.error({ ...err, message: err.message, stack: stackOnly }); + else + this.error(err); + } + + if (!pluginOptions.check) return - // workaround: err.stack contains err.message and Rollup prints both, causing duplication, so split out the stack itself if it exists (c.f. https://github.com/ezolenko/rollup-plugin-typescript2/issues/103#issuecomment-1172820658) - const stackOnly = err.stack?.split(err.message)[1]; - if (stackOnly) - this.error({ ...err, message: err.message, stack: stackOnly }); - else - this.error(err); + // type-check missed files as well + parsedConfig.fileNames.forEach((name) => + { + const key = normalize(name); + if (!filter(key)) + return; + + context.debug(() => `type-checking missed '${key}'`); + + const snapshot = servicesHost.getScriptSnapshot(key); + if (snapshot) + typecheckFile(key, snapshot, context); + }); }, generateBundle(bundleOptions) @@ -287,23 +304,6 @@ const typescript: PluginImpl = (options) => }); } - // type-check missed files as well - parsedConfig.fileNames.forEach((name) => - { - if (!pluginOptions.check) - return; - - const key = normalize(name); - if (key in declarations || !filter(key)) // don't duplicate if it's already been transformed - return; - - context.debug(() => `type-checking missed '${key}'`); - - const snapshot = servicesHost.getScriptSnapshot(key); - if (snapshot) - typecheckFile(key, snapshot, context); - }); - if (!watchMode && !noErrors) context.info(yellow("there were errors or warnings.")); From 5e6fc1edf75e54933fae6f18b70883d8b8321456 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Tue, 28 Jun 2022 18:26:58 -0400 Subject: [PATCH 3/8] add new checkedFiles Set to not duplicate type-checking - since `parsedConfig.fileNames` could include files that were already checked during the `transform` hook - and because `declarations` dict is empty when `declaration: false`, so can't check against that --- src/index.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/index.ts b/src/index.ts index 042f632f..1cd88001 100644 --- a/src/index.ts +++ b/src/index.ts @@ -33,6 +33,7 @@ const typescript: PluginImpl = (options) => let service: tsTypes.LanguageService; let noErrors = true; const declarations: { [name: string]: { type: tsTypes.OutputFile; map?: tsTypes.OutputFile } } = {}; + const checkedFiles = new Set(); let _cache: TsCache; const cache = (): TsCache => @@ -60,6 +61,8 @@ const typescript: PluginImpl = (options) => if (diagnostics.length > 0) noErrors = false; + + checkedFiles.add(id); } const pluginOptions: IOptions = Object.assign({}, @@ -141,6 +144,7 @@ const typescript: PluginImpl = (options) => { const key = normalize(id); delete declarations[key]; + checkedFiles.delete(key); }, resolveId(importee, importer) @@ -270,7 +274,7 @@ const typescript: PluginImpl = (options) => parsedConfig.fileNames.forEach((name) => { const key = normalize(name); - if (!filter(key)) + if (checkedFiles.has(key) || !filter(key)) // don't duplicate if it's already been checked return; context.debug(() => `type-checking missed '${key}'`); From 9ba4cedc9907ab99e4177eb4ee71b71ef42d16d3 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Fri, 1 Jul 2022 21:50:25 -0400 Subject: [PATCH 4/8] move checkedFiles.add to the beginning of typecheckFile - because printing diagnostics can bail if the category was error - that can result in a file being type-checked but not added to checkedFiles --- src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 1cd88001..4fa00e0d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -56,13 +56,13 @@ const typescript: PluginImpl = (options) => const typecheckFile = (id: string, snapshot: tsTypes.IScriptSnapshot, tcContext: IContext) => { + checkedFiles.add(id); // must come before print, as that could bail + const diagnostics = getDiagnostics(id, snapshot); printDiagnostics(tcContext, diagnostics, parsedConfig.options.pretty !== false); if (diagnostics.length > 0) noErrors = false; - - checkedFiles.add(id); } const pluginOptions: IOptions = Object.assign({}, From 0b98ab301fb9cc936a700fa26c4845831a9c2c49 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Fri, 1 Jul 2022 21:54:11 -0400 Subject: [PATCH 5/8] wip: fuse _ongenerate functionality into buildEnd, _onwrite into generateBundle - per ezolenko, the whole generateRound etc stuff was a workaround because the buildEnd hook actually _didn't exist_ before - so now that it does, we can use it to simplify some logic - no longer need `_ongenerate` as that should be in `buildEnd`, and no longer need `_onwrite` as it is the only thing called in `generateBundle`, so just combine them - importantly, `buildEnd` also occurs before output generation, so this ensures that type-checking still occurs even if `bundle.generate()` is not called - also move the `walkTree` call to above the "missed" type-checking as it needs to come first - it does unconditional type-checking once per watch cycle, whereas "missed" only type-checks those that were, well, "missed" - so in order to not have duplication, make "missed" come after, when the `checkedFiles` Set has been filled by `walkTree` already - and for simplification, just return early on error to match the current behavior - in the future, may want to print the error and continue type-checking other files - so that all type-check errors are reported, not just the first one NOTE: this is WIP because the `cache.done()` call and the `!noErrors` message are incorrectly blocked behind the `pluginOptions.check` conditional right now - `cache.done()` needs to be called regardless of check or error or not, i.e. in all scenarios - but ideally it should be called after all the type-checking here - `!noErrors` should be logged regardless of check or not - and similarly, after the type-checking --- src/index.ts | 48 ++++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/index.ts b/src/index.ts index 4fa00e0d..ec8af3f6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,6 +1,6 @@ import { relative, dirname, normalize as pathNormalize, resolve } from "path"; import * as tsTypes from "typescript"; -import { PluginImpl, PluginContext, InputOptions, OutputOptions, TransformResult, SourceMap, Plugin } from "rollup"; +import { PluginImpl, InputOptions, TransformResult, SourceMap, Plugin } from "rollup"; import { normalizePath as normalize } from "@rollup/pluginutils"; import { blue, red, yellow, green } from "colors/safe"; import findCacheDir from "find-cache-dir"; @@ -89,7 +89,7 @@ const typescript: PluginImpl = (options) => } setTypescriptModule(pluginOptions.typescript); - const self: Plugin & { _ongenerate: () => void, _onwrite: (this: PluginContext, _output: OutputOptions) => void } = { + const self: Plugin = { name: "rpt2", @@ -270,6 +270,20 @@ const typescript: PluginImpl = (options) => if (!pluginOptions.check) return + // walkTree once on each cycle when in watch mode + if (watchMode) + { + cache().walkTree((id) => + { + if (!filter(id)) + return; + + const snapshot = servicesHost.getScriptSnapshot(id); + if (snapshot) + typecheckFile(id, snapshot, context); + }); + } + // type-check missed files as well parsedConfig.fileNames.forEach((name) => { @@ -283,40 +297,18 @@ const typescript: PluginImpl = (options) => if (snapshot) typecheckFile(key, snapshot, context); }); - }, - - generateBundle(bundleOptions) - { - self._ongenerate(); - self._onwrite.call(this, bundleOptions); - }, - - _ongenerate(): void - { - context.debug(() => `generating target ${generateRound + 1}`); - - if (pluginOptions.check && watchMode && generateRound === 0) - { - cache().walkTree((id) => - { - if (!filter(id)) - return; - - const snapshot = servicesHost.getScriptSnapshot(id); - if (snapshot) - typecheckFile(id, snapshot, context); - }); - } if (!watchMode && !noErrors) context.info(yellow("there were errors or warnings.")); cache().done(); - generateRound++; }, - _onwrite(this: PluginContext, _output: OutputOptions): void + generateBundle(this, _output) { + context.debug(() => `generating target ${generateRound + 1}`); + generateRound++; + if (!parsedConfig.options.declaration) return; From c5bca6c2a20d7b13cfa3bf554064a9dc78740437 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Fri, 1 Jul 2022 22:15:33 -0400 Subject: [PATCH 6/8] call `cache().done()` and `!noErrors` in check and non-check conditions - instead of making a big `if` statement, decided to split out a `buildDone` function - to always call at the end of the input phase - we can also move the `cache().done()` in `emitSkipped` into `buildEnd`, as `buildEnd` gets called when an error occurs as well - and this way we properly print for errors as well - `buildDone` will have more usage in other PRs as well, so I figure it makes sense to split it out now as well --- src/index.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index ec8af3f6..1729ae51 100644 --- a/src/index.ts +++ b/src/index.ts @@ -65,6 +65,15 @@ const typescript: PluginImpl = (options) => noErrors = false; } + /** to be called at the end of Rollup's build phase, before output generation */ + const buildDone = (): void => + { + if (!watchMode && !noErrors) + context.info(yellow("there were errors or warnings.")); + + cache().done(); + } + const pluginOptions: IOptions = Object.assign({}, { check: true, @@ -204,9 +213,7 @@ const typescript: PluginImpl = (options) => { // always checking on fatal errors, even if options.check is set to false typecheckFile(id, snapshot, contextWrapper); - // since no output was generated, aborting compilation - cache().done(); this.error(red(`failed to transpile '${id}'`)); } @@ -259,6 +266,7 @@ const typescript: PluginImpl = (options) => { if (err) { + buildDone(); // workaround: err.stack contains err.message and Rollup prints both, causing duplication, so split out the stack itself if it exists (c.f. https://github.com/ezolenko/rollup-plugin-typescript2/issues/103#issuecomment-1172820658) const stackOnly = err.stack?.split(err.message)[1]; if (stackOnly) @@ -268,7 +276,7 @@ const typescript: PluginImpl = (options) => } if (!pluginOptions.check) - return + return buildDone(); // walkTree once on each cycle when in watch mode if (watchMode) @@ -298,10 +306,7 @@ const typescript: PluginImpl = (options) => typecheckFile(key, snapshot, context); }); - if (!watchMode && !noErrors) - context.info(yellow("there were errors or warnings.")); - - cache().done(); + buildDone(); }, generateBundle(this, _output) From 3a027ca324248c08dacaa04cd8420d91cc2f39dd Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Wed, 6 Jul 2022 17:58:15 -0400 Subject: [PATCH 7/8] use `RollupContext` for type-only files - i.e. bail out when `abortOnError: true`, which `ConsoleContext` can't do - `ConsoleContext` is basically meant for everywhere `RollupContext` can't be used - which is effectively only in the `options` hook, per the Rollup docs: https://rollupjs.org/guide/en/#options --- src/index.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/index.ts b/src/index.ts index 1729ae51..f4b57a2c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -292,6 +292,8 @@ const typescript: PluginImpl = (options) => }); } + const contextWrapper = new RollupContext(pluginOptions.verbosity, pluginOptions.abortOnError, this, "rpt2: "); + // type-check missed files as well parsedConfig.fileNames.forEach((name) => { @@ -303,7 +305,7 @@ const typescript: PluginImpl = (options) => const snapshot = servicesHost.getScriptSnapshot(key); if (snapshot) - typecheckFile(key, snapshot, context); + typecheckFile(key, snapshot, contextWrapper); }); buildDone(); From 01373c1264b28a34bf813a26142f23f2cfc74970 Mon Sep 17 00:00:00 2001 From: Anton Gilgur Date: Wed, 6 Jul 2022 21:05:38 -0400 Subject: [PATCH 8/8] add test for type-only file with type errors - now that the integration tests exist, we can actually test this scenario - refactor: give each test their own `onwarn` mock when necessary - while `restoreMocks` is set in the `jest.config.js`, Jest apparently has poor isolation of mocks: https://github.com/facebook/jest/issues/7136 - if two tests ran in parallel, `onwarn` was getting results from both, screwing up the `toBeCalledTimes` number - couldn't get the syntax error to work with `toBeCalledTimes` either - if no mock is given, it _does_ print warnings, but if a mock is given, it doesn't print, yet isn't called? - I think the mock is getting screwed up by the error being thrown here; maybe improperly saved or something --- __tests__/integration/errors.spec.ts | 44 ++++++++++++++++--- .../fixtures/errors/import-type-error.ts | 8 ++++ .../errors/type-only-import-with-error.ts | 2 + 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 __tests__/integration/fixtures/errors/import-type-error.ts create mode 100644 __tests__/integration/fixtures/errors/type-only-import-with-error.ts diff --git a/__tests__/integration/errors.spec.ts b/__tests__/integration/errors.spec.ts index cc40cbe1..002895b3 100644 --- a/__tests__/integration/errors.spec.ts +++ b/__tests__/integration/errors.spec.ts @@ -1,4 +1,5 @@ import { jest, afterAll, test, expect } from "@jest/globals"; +import { Mock } from "jest-mock" import * as path from "path"; import { normalizePath as normalize } from "@rollup/pluginutils"; import * as fs from "fs-extra"; @@ -12,7 +13,6 @@ jest.setTimeout(15000); const local = (x: string) => path.resolve(__dirname, x); const cacheRoot = local("__temp/errors/rpt2-cache"); // don't use the one in node_modules -const onwarn = jest.fn(); afterAll(async () => { // workaround: there seems to be some race condition causing fs.remove to fail, so give it a sec first (c.f. https://github.com/jprichardson/node-fs-extra/issues/532) @@ -20,7 +20,7 @@ afterAll(async () => { await fs.remove(cacheRoot); }); -async function genBundle(relInput: string, extraOpts?: RPT2Options) { +async function genBundle(relInput: string, extraOpts?: RPT2Options, onwarn?: Mock) { const input = normalize(local(`fixtures/errors/${relInput}`)); return helpers.genBundle({ input, @@ -43,9 +43,10 @@ test("integration - semantic error", async () => { }); test("integration - semantic error - abortOnError: false / check: false", async () => { + const onwarn = jest.fn(); // either warning or not type-checking should result in the same bundle - const { output } = await genBundle("semantic.ts", { abortOnError: false }); - const { output: output2 } = await genBundle("semantic.ts", { check: false }); + const { output } = await genBundle("semantic.ts", { abortOnError: false }, onwarn); + const { output: output2 } = await genBundle("semantic.ts", { check: false }, onwarn); expect(output).toEqual(output2); expect(output[0].fileName).toEqual("index.js"); @@ -60,7 +61,38 @@ test("integration - syntax error", () => { }); test("integration - syntax error - abortOnError: false / check: false", () => { + const onwarn = jest.fn(); const err = "Unexpected token (Note that you need plugins to import files that are not JavaScript)"; - expect(genBundle("syntax.ts", { abortOnError: false })).rejects.toThrow(err); - expect(genBundle("syntax.ts", { check: false })).rejects.toThrow(err); + expect(genBundle("syntax.ts", { abortOnError: false }, onwarn)).rejects.toThrow(err); + expect(genBundle("syntax.ts", { check: false }, onwarn)).rejects.toThrow(err); +}); + +const typeOnlyIncludes = ["**/import-type-error.ts", "**/type-only-import-with-error.ts"]; + +test("integration - type-only import error", () => { + expect(genBundle("import-type-error.ts", { + include: typeOnlyIncludes, + })).rejects.toThrow("Property 'nonexistent' does not exist on type 'someObj'."); +}); + +test("integration - type-only import error - abortOnError: false / check: false", async () => { + const onwarn = jest.fn(); + // either warning or not type-checking should result in the same bundle + const { output } = await genBundle("import-type-error.ts", { + include: typeOnlyIncludes, + abortOnError: false, + }, onwarn); + const { output: output2 } = await genBundle("import-type-error.ts", { + include: typeOnlyIncludes, + 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 + expect(onwarn).toBeCalledTimes(1); }); diff --git a/__tests__/integration/fixtures/errors/import-type-error.ts b/__tests__/integration/fixtures/errors/import-type-error.ts new file mode 100644 index 00000000..d6f46a1b --- /dev/null +++ b/__tests__/integration/fixtures/errors/import-type-error.ts @@ -0,0 +1,8 @@ +// this file has no errors itself; it is used an entry file to test an error in a type-only import + +export type { typeError } from "./type-only-import-with-error"; + +// some code so this file isn't empty +export function sum(a: number, b: number) { + return a + b; +} diff --git a/__tests__/integration/fixtures/errors/type-only-import-with-error.ts b/__tests__/integration/fixtures/errors/type-only-import-with-error.ts new file mode 100644 index 00000000..239cdadf --- /dev/null +++ b/__tests__/integration/fixtures/errors/type-only-import-with-error.ts @@ -0,0 +1,2 @@ +type someObj = {}; +export type typeError = someObj['nonexistent'];