Skip to content

Commit

Permalink
fix: type-check included files missed by transform (type-only fil…
Browse files Browse the repository at this point in the history
…es) (#345)

* 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))

* 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

* 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

* 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

* 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

* 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

* 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

* 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: jestjs/jest#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
  • Loading branch information
agilgur5 committed Jul 12, 2022
1 parent af271af commit eab48a3
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 35 deletions.
44 changes: 38 additions & 6 deletions __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";
Expand All @@ -11,15 +12,14 @@ 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)
await new Promise(resolve => setTimeout(resolve, 1000));
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,
Expand All @@ -42,9 +42,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");
Expand All @@ -59,7 +60,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);
});
8 changes: 8 additions & 0 deletions __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;
}
@@ -0,0 +1,2 @@
type someObj = {};
export type typeError = someObj['nonexistent'];
78 changes: 49 additions & 29 deletions 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";
Expand Down Expand Up @@ -33,6 +33,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
let service: tsTypes.LanguageService;
let noErrors = true;
const declarations: { [name: string]: { type: tsTypes.OutputFile; map?: tsTypes.OutputFile } } = {};
const checkedFiles = new Set<string>();

let _cache: TsCache;
const cache = (): TsCache =>
Expand All @@ -55,13 +56,24 @@ const typescript: PluginImpl<RPT2Options> = (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;
}

/** 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,
Expand All @@ -86,7 +98,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
}
setTypescriptModule(pluginOptions.typescript);

const self: Plugin & { _ongenerate: () => void, _onwrite: (this: PluginContext, _output: OutputOptions) => void } = {
const self: Plugin = {

name: "rpt2",

Expand Down Expand Up @@ -141,6 +153,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
{
const key = normalize(id);
delete declarations[key];
checkedFiles.delete(key);
},

resolveId(importee, importer)
Expand Down Expand Up @@ -201,9 +214,7 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
noErrors = false;
// 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}'`));
}

Expand Down Expand Up @@ -254,28 +265,22 @@ const typescript: PluginImpl<RPT2Options> = (options) =>

buildEnd(err)
{
if (!err)
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);
},

generateBundle(bundleOptions)
{
self._ongenerate();
self._onwrite.call(this, bundleOptions);
},
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)
this.error({ ...err, message: err.message, stack: stackOnly });
else
this.error(err);
}

_ongenerate(): void
{
context.debug(() => `generating target ${generateRound + 1}`);
if (!pluginOptions.check)
return buildDone();

if (pluginOptions.check && watchMode && generateRound === 0)
// walkTree once on each cycle when in watch mode
if (watchMode)
{
cache().walkTree((id) =>
{
Expand All @@ -288,15 +293,30 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
});
}

if (!watchMode && !noErrors)
context.info(yellow("there were errors or warnings."));
const contextWrapper = new RollupContext(pluginOptions.verbosity, pluginOptions.abortOnError, this, "rpt2: ");

cache().done();
generateRound++;
// type-check missed files as well
parsedConfig.fileNames.forEach((name) =>
{
const key = normalize(name);
if (checkedFiles.has(key) || !filter(key)) // don't duplicate if it's already been checked
return;

context.debug(() => `type-checking missed '${key}'`);

const snapshot = servicesHost.getScriptSnapshot(key);
if (snapshot)
typecheckFile(key, snapshot, contextWrapper);
});

buildDone();
},

_onwrite(this: PluginContext, _output: OutputOptions): void
generateBundle(this, _output)
{
context.debug(() => `generating target ${generateRound + 1}`);
generateRound++;

if (!parsedConfig.options.declaration)
return;

Expand Down

0 comments on commit eab48a3

Please sign in to comment.