Non-deterministic error when compiling files outside of the TS project

[davazp]

We have hit a strange bug that would fail rarely (not deterministically) a build. When it failed, it threw the following error:

ERROR in ./packages/bar/bar.ts
Module build failed (from ../index.js):
Error: TypeScript emitted no output for /Users/davazp/Projects/ts-loader/minimal-repo/packages/bar/bar.ts.
    at makeSourceMapAndFinish (/Users/davazp/Projects/ts-loader/dist/index.js:99:15)
    at successLoader (/Users/davazp/Projects/ts-loader/dist/index.js:89:9)
    at retry (/Users/davazp/Projects/ts-loader/dist/index.js:35:17)
    at Object.loader (/Users/davazp/Projects/ts-loader/dist/index.js:42:5)
 @ ./packages/foo/foo.ts 3:12-30

The cause turned out to be that such file was not part of the include section of the tsconfig.json. However, this non-deterministic behaviour is really confusing and undesirable, so I went deeper into the causes to really understand the problem and help to improve it. See the Analysis section below.

Expected Behaviour

I think ts-loader should have thrown an error when webpack passes a file to it that is not part of the TS project. This seems to me the most sensible solution. That way tsc and other TS-based tools will behave in the same way.

Actual Behaviour

The build will work or fail depending on the order in which webpack processes the files, which is not deterministic even without making any changes to the files.

Steps to Reproduce the Problem

It's not easy to reproduce because it depends on the order of the files. To reproduce it, I hacked ts-loader to enforce the order in a test project. To try:

• clone https://github.com/davazp/ts-loader
• Build it as usual, and in the minimal-repo/ directory
• yarn
• yarn build

You should get the error described at the beginning of the issue.

Location of a Minimal Repository that Demonstrates the Issue.

This shows the changes I did to ts-loader to reproduce the issue:
master...davazp:ts-loader-issue

Analysis

After a lot of hard debugging, I managed to figure out what was going on.

The setup (like in the example repo) is

packages/foo/foo.ts   # this file is included in tsconfig.json
packages/bar/bar.ts   # this file is not

On a failed build:

1. The order in which webpack provides the files to ts-loader is not deterministic.
   Two consequent builds could process files in a different order. I instrumented ts-loader
   to record the order on failure, and enforced the order to reproduce the problem.

2. When foo/foo.ts is compiled, bar/bar.ts is added to instance.files because it is a dependency as a result to a call to getScriptSnapshot from TS. Additionally, it is marked as a external dependency because it is under node_modules in the TS' program.

3. Later on, bar/bar.ts is found as a toplevel file. updateFileInCache thinks it is already a root file because it is in instance.files, so instance.version is not increased.

   As a consequence, TS reuses the program and the map sourceFilesFoundSearchingNodeModules is not thrown away. As an external file, Typescript will not emit any output for it.

This could work if finding a new root level file (even if it was found as a dependency before) increases the instance.version, but then ts-loader would deviate from other TS tools.

Why does it work for other file ordering?

If between 2) and 3), another file outside of the project is processed, which is not a dependency of any other file, then the file is not present in instance.files, so updateFileInCache will add it and increase instance.version, triggering a discard of the file is properly added as a new root file (as it is not in instance.files). This cause an instance.version increase and flushing the sourceFilesFoundSearchingNodeModules variable.

[davazp]

If that is an acceptable solution and would not break anything else, I can work on a pull request . 🙂 Thanks for your work!

[johnnyreilly]

First of all, thanks for your detailed work and analysis. I really appreciate the effort you've gone to 🥰

Things we like:

1. Determinism. Obvs 😁
2. Where it makes sense, ts-loader aims to be a drop in replacement for tsc. You're suggesting more of this and that very much aligns with our goals.

However, file resolution is actually one of the areas where we may differ behaviour wise. I think. Alas I can't remember the gory details but I'm going to trust in our tests that they should reveal any required differences in behaviour.

I'm interested in the problem you've outlined. If there's a way to come up with a performant remedy that doesn't break existing use cases then I think it could be worth looking to make changes.

If you're willing to work on this I'm happy to provide feedback and see where this leads. My advice is fork and get hacking. Don't worry about getting passing tests in place before submitting a PR - some of our tests are quirky and I can likely advise if there are issues.

It's worth saying that this may be one of the areas which if we make changes may reveal hidden problems. There's include, exclude and files all playing their part. I'm open to finding out if you are. 😉

[andrewbranch]

@davazp I’ve assigned you so it’s visible that this is being actively worked on; it does not imply a contractual obligation 😛