ezolenko/rollup-plugin-typescript2

Vague `failed to transpile` error -- `noEmitOnError: true` breaks rpt2

benmccann opened this issue · 7 comments

benmccann commented

What happens and why it is wrong

I get the error:

[!] (plugin rpt2) Error: failed to transpile 'packages/kit/src/runtime/navigation/index.ts'

This is almost impossible to debug. Luckily I figured out I can do npx tsc and get the real error message:

src/runtime/navigation/types.ts:33:9 - error TS2304: Cannot find name 'Route'.

33  route: Route;
           ~~~~~


Found 1 error.

Environment

Versions
npx: installed 1 in 1.212s

  npmPackages:
    rollup: ^2.32.0 => 2.32.0 
    rollup-plugin-typescript2: ^0.29.0 => 0.29.0 
    typescript: ^4.0.3 => 4.0.3

Steps to reproduce

If you do not have pnpm then first run npm install -g pnpm

git clone git@github.com:sveltejs/kit.git
cd kit
git reset --hard 2483ae3730a7219814cc654c03b0409a797de2ee
pnpm install
pnpm -r build

After this, you can build just the relevant sub-project by running cd packages/kit and then npx rollup -c for faster debugging

Other context

Looks like it's coming from here:

rollup-plugin-typescript2/src/index.ts

Line 221 in 6fb0e75

this.error(red(`failed to transpile '${id}'`));

printDiagnostics right above looks to be being passed an empty array of diagnostics

agilgur5 commented

Per the downstream issue, it seems like OP believed this might be caused by #234 , with pnpm somehow swallowing the diagnostic errors

agilgur5 commented

I fixed pnpm (and symlinks in general) in #332, so I re-visited this to see if it solves the issue here, since pnpm was mentioned downstream.

It was a good thing that the commit hash was included in the issue so that I could go back in time to the right spot.

I tried upgrading rpt2 to latest and added the fix in #332 locally to node_modules, but neither of those fixed it, errors were still not reported. I removed some of the rpt2 config to simplify things and added clean: true (in case of a cache issue), but those changes didn't fix it either. I also set verbosity: 3 (which the issue template requests the log from) and indeed no diagnostics are printed before this error. The diagnostics array is also indeed empty 🤔

So, as far as I can tell, I don't think this is related to #234 , as the fix for it does not fix this issue, unfortunately.

agilgur5 commented

Investigated a little bit more and both semantic and syntactic diagnostics return empty. These are provided by the TS LanguageService, so not something that rpt2 has any role in.

Checked one step out, the snapshot, and I found:

snapshot:  StringScriptSnapshot {
  text: "export { default as goto } from './goto';\n" +
    "export { default as prefetch } from './prefetch';\n" +
    "export { default as prefetchRoutes } from './prefetchRoutes';\n" +
    "export { default as start } from './start';"
}

from id kit/packages/kit/src/runtime/navigation/index.ts.

That is indeed what that file looks like and there are indeed no errors in that file... so the lack of diagnostic output is correct in that case.

The error from tsc that OP mentioned comes from kit/packages/kit/src/runtime/navigation/types.ts, which is a transitive import of index.ts (some of the files it imports then import types.ts).

So what this suggests the problem is is that on a fatal error, only the first file reported on will show diagnostics, and not its transitive imports that may actually be the cause of the error.

Annnd this seems to be another case where the lacking TS Compiler docs have no mention or example of this case what-so-ever. So unsurprising that rpt2 doesn't handle it given that it's not even mentioned in the docs. rpt2 handles it the same way as the docs do 😕

EDIT: See below, this is actually due to noEmitOnError: true (which well, also isn't mentioned in TS Compiler docs, but none of the overrides a plugin like rpt2 needs are for that matter)

agilgur5 commented

One more try and I seem to have found root cause finally!
The problem is with the noEmitOnError: true setting in your tsconfig. This defaults to false.

When this is removed, rpt2 passes, and your adjust-typings plugin errors instead.

rpt2 has the setting abortOnError for this (which may duplicate #62), which actually defaults to true because skipping a TS error may cause even more confusing Rollup errors down-the-line (since it's a pipeline, not just a single parser like tsc).

There's also an old, stale feature request, #168 , that requests all errors be reported before aborting, instead of just the first error. That seems related/like what this might duplicate. Although I actually think your use-case is already handled and doesn't require that, as tsc only reports one type-error anyway.

That being said, since rpt2 defaults abortOnError to true, you would think removing noEmitOnError: true wouldn't make a difference, but it actually does in 2 ways:

  1. noEmitOnError is an emit setting, and so it acts like noEmit when there's an error: i.e. all files will have emitSkipped set to true, which is how rpt2 currently determines if an error is fatal or not.
    So we'll want to force noEmitOnError to false actually. We effectively do this anyway since abortOnError is true by default and doesn't relate to the noEmitOnError setting, it's just that the noEmit portion was probably not expected to break things like this and is very infrequently set in tsconfigs, so hasn't popped up till now.

  2. types.ts is a type-only / emit-less TS file, so rpt2 passes and actually doesn't error out at all when noEmitOnError is removed. That is due to #211 and other long-standing type-only issues

So once 1. is fixed, this will duplicate #211

agilgur5 commented

Forcing noEmitOnError: false has been released in 0.32.0!

For type-only imports, refer to #211 for progress

agilgur5 commented

Actually, the type-only portion of this should be fixed by #345 as this use-case was using tsconfig include globs (duplicating #298). There's more fixes to come for different situations with type-only files though.

agilgur5 commented

#345 has been released in 0.33.0

  • 👍1
  • 🎉1