Original TS internal error is hidden

[simonbuchan]

Pulled out a more specific request from #793

Expected Behaviour

When hitting (certain) internal TS errors such as microsoft/TypeScript#24932 (TS 2.9.1) and microsoft/TypeScript#26554 (TS 3.0.1), ts-loader attempts to get the error diagnostics which can fail with a stack like:

/Users/mehdibenadda/Development/fun-cms/node_modules/typescript/lib/typescript.js:107386
            return program.getOptionsDiagnostics(cancellationToken).concat(program.getGlobalDiagnostics(cancellationToken));
                           ^

TypeError: Cannot read property 'getOptionsDiagnostics' of undefined
    at Object.getCompilerOptionsDiagnostics (/Users/mehdibenadda/Development/fun-cms/node_modules/typescript/lib/typescript.js:107386:28)
    at provideCompilerOptionDiagnosticErrorsToWebpack (/Users/mehdibenadda/Development/fun-cms/node_modules/ts-loader/dist/after-compile.js:39:31)
    at /Users/mehdibenadda/Development/fun-cms/node_modules/ts-loader/dist/after-compile.js:17:9
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/mehdibenadda/Development/fun-cms/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:16:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/Users/mehdibenadda/Development/fun-cms/node_modules/tapable/lib/Hook.js:35:21)
    at compilation.seal.err (/Users/mehdibenadda/Development/fun-cms/node_modules/webpack/lib/Compiler.js:497:30)
    at AsyncSeriesHook.eval [as callAsync] (eval at create (/Users/mehdibenadda/Development/fun-cms/node_modules/tapable/lib/HookCodeFactory.js:24:12), <anonymous>:6:1)
    at AsyncSeriesHook.lazyCompileHook [as _callAsync] (/Users/mehdibenadda/Development/fun-cms/node_modules/tapable/lib/Hook.js:35:21)
    at hooks.optimizeAssets.callAsync.err (/Users/mehdibenadda/Development/fun-cms/node_modules/webpack/lib/Compilation.js:985:35)
...

Actual Behaviour

The original TS error is reported, e.g. for microsoft/TypeScript#26554

/Users/fabio/projects/0x-monorepo-one/node_modules/typescript/lib/typescript.js:14080
        return path.replace(backslashRegExp, ts.directorySeparator);
                    ^

TypeError: Cannot read property 'replace' of undefined
    at Object.normalizeSlashes (/Users/fabio/projects/0x-monorepo-one/node_modules/typescript/lib/typescript.js:14080:21)
    at getCommonSourceDirectory (/Users/fabio/projects/0x-monorepo-one/node_modules/typescript/lib/typescript.js:82737:68)
    at verifyCompilerOptions (/Users/fabio/projects/0x-monorepo-one/node_modules/typescript/lib/typescript.js:84278:27)
    at Object.createProgram (/Users/fabio/projects/0x-monorepo-one/node_modules/typescript/lib/typescript.js:82703:9)
    at Converter.convert (/Users/fabio/projects/0x-monorepo-one/node_modules/typedoc/dist/lib/converter/converter.js:109:26)
    at CliApplication.Application.convert (/Users/fabio/projects/0x-monorepo-one/node_modules/typedoc/dist/lib/application.js:84:37)
    at CliApplication.bootstrap (/Users/fabio/projects/0x-monorepo-one/node_modules/typedoc/dist/lib/cli.js:62:32)
    at CliApplication.Application [as constructor] (/Users/fabio/projects/0x-monorepo-one/node_modules/typedoc/dist/lib/application.js:44:15)
    at new CliApplication (/Users/fabio/projects/0x-monorepo-one/node_modules/typedoc/dist/lib/cli.js:38:42)
    at Object.<anonymous> (/Users/fabio/projects/0x-monorepo-one/node_modules/typedoc/bin/typedoc:4:1)
error Command failed with exit code 1.

Steps to Reproduce the Problem

For microsoft/TypeScript#26554, it should be enough to have a ts-loader config that looks something like:

{
  test: /\.tsx?$/,
  loader: 'ts-loader',
  options: {
    compilerOptions: {
      composite: true,
    }
  }
}

i.e. compiler options has composite, but not rootDir set.

Location of a Minimal Repository that Demonstrates the Issue.

microsoft/TypeScript#26554 (comment) links to a reproducing case.

[johnnyreilly]

Thanks for the detailed issue; that's always helpful. Do you have any insight as to what you feel a sensible workaround would look like?

[simonbuchan]

I was a bit stymied figuring out where the original error got swallowed, the debugger stops in what looks like regenerator-runtime code with no user code on the stack. My guess is that either you or webpack are catching the exception to be reported later, but this hook is being fired first. If so, adding a flag tracking that an error was thrown so you know not to call back in should be enough?

[johnnyreilly]

Just had a thought. Is this specifically related to projectReferences functionality? (Hence composite: true?)

That's not supported in ts-loader yet, but hopefully will be: #817

[simonbuchan]

No, #793 was originally caused by a TS 2.9 bug microsoft/TypeScript#24932

It's true the case I care about is triggered when the target project is composite (and rootDir is not set), but that's not strictly a problem for ts-loader: composite makes a project reference-able (and enforces the options that that requires), not that it references other projects. (Which you might do even for "root" app projects if you use the TS handbook recommended structure of having an empty src/tsconfig.json that references all of your src/someproject/tsconfig.json so you can watch / build all your projects with one command).

[johnnyreilly]

Which you might do even for "root" app projects if you use the TS handbook recommended structure of having an empty src/tsconfig.json that references all of your src/someproject/tsconfig.json so you can watch / build all your projects with one command

Could you share a link to that please? I'd love to read.

My guess is that either you or webpack are catching the exception to be reported later,

Roughly speaking you can track where errors are propogated by looking for module.errors.push:

https://github.com/TypeStrong/ts-loader/search?utf8=✓&q=_module.errors.push&type=

Does that help?

If so, adding a flag tracking that an error was thrown so you know not to call back in should be enough?

Why don't you submit a PR to show me what you have in mind? Don't worry about tests for now; just something to illustrate what you're thinking of. We can use that as a basis for discussion / potential implementation.

[simonbuchan]

Here's the handbook page on project references. The structure I mentioned is described in the "Guidance" section at the bottom.

Messed around with the code for a bit, here's a hacky-looking fix:

diff --git i/src/after-compile.ts w/src/after-compile.ts
index 5ba828d..8e71b73 100644
--- i/src/after-compile.ts
+++ w/src/after-compile.ts
@@ -19,6 +19,10 @@ export function makeAfterCompile(
   let checkAllFilesForErrors = true;

   return (compilation: WebpackCompilation, callback: () => void) => {
+    if (compilation.errors.length) {
+      callback();
+      return;
+    }
     // Don't add errors for child compilations
     if (compilation.compiler.isChild()) {
       callback();

Which gives the desired root error:

$ /home/simon/temp/ts-loader/examples/vanilla/node_modules/.bin/webpack --mode none
ts-loader: Using typescript@3.0.1 and /home/simon/temp/ts-loader/examples/vanilla/src/client/tsconfig.json
Hash: 1e9bcff6792a56c02f7e
Version: webpack 4.17.1
Time: 688ms
Built at: 08/29/2018 8:05:42 PM
  Asset      Size  Chunks             Chunk Names
main.js  4.64 KiB       0  [emitted]  main
[0] ./src/client/index.ts 1.07 KiB {0} [built] [failed] [1 error]

ERROR in ./src/client/index.ts
Module build failed (from /home/simon/temp/ts-loader/index.js):
TypeError: Cannot read property 'replace' of undefined
    at Object.normalizeSlashes (/home/simon/temp/ts-loader/node_modules/typescript/lib/typescript.js:14080:21)
    at getCommonSourceDirectory (/home/simon/temp/ts-loader/node_modules/typescript/lib/typescript.js:82737:68)
    at verifyCompilerOptions (/home/simon/temp/ts-loader/node_modules/typescript/lib/typescript.js:84278:27)
    at Object.createProgram (/home/simon/temp/ts-loader/node_modules/typescript/lib/typescript.js:82703:9)
    at synchronizeHostData (/home/simon/temp/ts-loader/node_modules/typescript/lib/typescript.js:111664:26)
    at Object.getEmitOutput (/home/simon/temp/ts-loader/node_modules/typescript/lib/typescript.js:111946:13)
    at Object.getEmitOutput (/home/simon/temp/ts-loader/dist/instances.js:187:41)
    at getEmit (/home/simon/temp/ts-loader/dist/index.js:196:37)
    at successLoader (/home/simon/temp/ts-loader/dist/index.js:34:11)
    at Object.loader (/home/simon/temp/ts-loader/dist/index.js:21:12)

So webpack was the one catching the error and adding it to the compilation errors list (which makes sense), but when the hook threw, it essentially replaced the error list (which is pretty lame).

Not quite sure what the right behavior is here, a slight better hack fix would be to catch any errors in the hook and push them to the errors list, but I think the right thing to do is to try to somehow distinguish non-exceptional errors (like user TS errors) and internal errors which might indicate that the compiler is "poisoned", but I don't know what that would look like.

Also, it does look like microsoft/TypeScript#26554 requires the project requires the target project to have both composite and references, but not rootDir. but that's a different issue!

[johnnyreilly]

Please bear with me as I'm away from home right now with only access to my phone and poor connectivity. Debugging in my head is difficult!

a slight better hack fix would be to catch any errors in the hook and push them to the errors list,

Isn't this what already happens? Or rather, reading through the hook logic I see the old TypeScript errors being removed but I don't see this happening:

it essentially replaced the error list

I don't follow how that happens?

[simonbuchan]

It's webpack that essentially replaces, by printing the new error, but not the error list.

When I say "error" here, I mean a thrown exception, not a result returned from the ts error apis. Sorry, this is a bit confusing to talk about!

[simonbuchan]

Added a PR if you missed it; in case that's easier for you?

[johnnyreilly]

Thanks I'll take a look

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

Closing as stale. Please reopen if you'd like to work on this further.