Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Original TS internal error is hidden #824

Closed
simonbuchan opened this issue Aug 27, 2018 · 12 comments
Closed

Original TS internal error is hidden #824

simonbuchan opened this issue Aug 27, 2018 · 12 comments
Labels

Comments

@simonbuchan
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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!

simonbuchan added a commit to simonbuchan/ts-loader that referenced this issue Aug 29, 2018
@johnnyreilly
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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

@johnnyreilly
Copy link
Member

Thanks I'll take a look

@stale
Copy link

stale bot commented Jan 19, 2019

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 stale bot added the wontfix label Jan 19, 2019
@stale
Copy link

stale bot commented Jan 26, 2019

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

@stale stale bot closed this as completed Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants