TypeStrong / ts-loader

TypeScript loader for webpack
https://johnnyreilly.com/ts-loader-goes-webpack-5
MIT License
3.46k stars 431 forks source link

Other loaders are not applied to unchanged files in watch mode #1111

Closed iorate closed 4 years ago

iorate commented 4 years ago

Expected Behaviour

In watch mode, unchanged .ts files should be passed to loaders other than ts-loader, as in v6.2.2.

Actual Behaviour

Unchanged .ts files are not passed to other loaders.

Steps to Reproduce the Problem

Please see the below repository. The version of ts-loader is 7.0.4.

./app.ts and ./error.ts are transpiled using ./remove-error-loader.js and ts-loader (in this order). ./error.ts has an "error", but it is removed by ./remove-error-loader.js. When I run npx webpack --watch, it seems to be working well at first:

% npx webpack --watch

webpack is watching the files…

Hash: f695893648cef87b9d90
Version: webpack 4.43.0
Time: 981ms
Built at: 2020/05/17 22:18:19
    Asset      Size  Chunks             Chunk Names
bundle.js  8.93 KiB    main  [emitted]  main
Entrypoint main = bundle.js
[./app.ts] 90 bytes {main} [built]
[./error.ts] 0 bytes {main} [built]

However, when I update ./app.ts (for example, change 'Hello' to 'Hello, world!'), an error occurs:

Hash: a3f00bec5e5882c9d219
Version: webpack 4.43.0
Time: 39ms
Built at: 2020/05/17 22:18:25
    Asset      Size  Chunks             Chunk Names
bundle.js  8.95 KiB    main  [emitted]  main
Entrypoint main = bundle.js
[./app.ts] 98 bytes {main} [built]
    + 1 hidden module

ERROR in C:\Users\iorate\ts-loader-error\error.ts
./error.ts
[tsl] ERROR in C:\Users\iorate\ts-loader-error\error.ts(1,1)
      TS2552: Cannot find name 'error'. Did you mean 'Error'?

This means that ./error.ts (unchanged) is compiled by a TypeScript compiler, but not processed by ./remove-error-loader.js before that.

With ts-loader@6.2.2, an update of ./app.ts does not cause an error:

Hash: 412257a27b607df16d5e
Version: webpack 4.43.0
Time: 29ms
Built at: 2020/05/17 22:16:30
    Asset      Size  Chunks             Chunk Names
bundle.js  8.95 KiB    main  [emitted]  main
Entrypoint main = bundle.js
[./app.ts] 98 bytes {main} [built]
    + 1 hidden module

Is this regression, or expected behavior in v7?

Location of a Minimal Repository that Demonstrates the Issue.

https://github.com/iorate/ts-loader-error

johnnyreilly commented 4 years ago

What an interesting use case! I didn't know that people were doing this - what do you use this behaviour for?

I think we'd be open to pull requests to reintroduce this behaviour if you wanted to take a look at this?

iorate commented 4 years ago

@johnnyreilly I preprocess TypeScript code by ifdef-loader to generate different build variants. Code before being preprocessed can be invalid in TypeScript, for example, https://github.com/iorate/uBlacklist/blob/58a4816d9ae74fd6507ca419c080271d8f86a46b/src/scripts/background/token.ts#L6-L10 so causes an error in watch mode with ts-loader@7.0.4 as described above.

The behavior changed at v7.0.1 (https://github.com/TypeStrong/ts-loader/commit/cc312879cdd252e27eb7bfc1073f5d2efc623efb). Reverting this will fix my problem, but the problem that @Zn4rK had will recur. It is difficult for me to find a way to solve both of the problems. I don't understand why removal of filePath.match(constants.tsTsxJsJsxRegex) !== null changes the behavior. Could you please help me?

johnnyreilly commented 4 years ago

This is very mysterious. Could you do some debugging please? I'm interested in knowing which files are coming through that the regex change is relevant to. Given that the example you've given is a TypeScript file: https://github.com/iorate/uBlacklist/blob/58a4816d9ae74fd6507ca419c080271d8f86a46b/src/scripts/background/token.ts#L6-L10

I would expect this to pass the regex and behaviour to be identical. Do you want to put some console.logs in place and investigate what files will experience different behaviour before and after the change please?

iorate commented 4 years ago

Sorry, I was mistaken. The important point of the commit (https://github.com/TypeStrong/ts-loader/commit/cc312879cdd252e27eb7bfc1073f5d2efc623efb) is not the removal of the regex, but the "inversion" of timestamp comparison. Actually re-inverting date <= lastTime to date > lastTime fixes my problem.

I explored the history of watch-run.ts:

The commit that introduced timestamp comparison was https://github.com/TypeStrong/ts-loader/commit/44a2cdb95075b59a49d9a2b19630dbfc564d909d :

Object.keys(times)
    .filter(filePath =>
        times[filePath] > (lastTimes[filePath] || startTime)
        && !!filePath.match(constants.tsTsxJsJsxRegex)
    )
    .forEach(filePath => {
         lastTimes[filePath] = times[filePath];
         // ...
     });

The commit https://github.com/TypeStrong/ts-loader/commit/388f2a617d1dbdbace574b2bf0bcdd933d440e0a , probably mistakenly, inverted the comparison:

for (const [filePath, date] of times) {
    if (date > (lastTimes.get(filePath) || startTime)
        && filePath.match(constants.tsTsxJsJsxRegex)) {
        continue;
    }

    lastTimes.set(filePath, date);

    updateFile(instance, filePath);
}

.filter( > ).forEach(...) was wrongly transformed to for () { if ( > ) continue; ... }.

Finally, https://github.com/TypeStrong/ts-loader/commit/cc312879cdd252e27eb7bfc1073f5d2efc623efb fixed it.

for (const [filePath, date] of times) {
    const lastTime = lastTimes.get(filePath) || startTime;

    if (date <= lastTime) {
        continue;
    }

    lastTimes.set(filePath, date);
    updateFile(instance, filePath);
}

So the current behavior seems to be initially intended.

I guess the current behavior is that: when a TypeScript file is changed, it is passed to tsc, and tsc loads all files including unchanged ones referring to tsconfig.json (right?).

However, even though unchanged files are passed to tsc, they should not be directly passed when other loaders are specified, IMHO.

Zn4rK commented 4 years ago

Hmm. Interesting. I think that the problem here is that we're reading the file from the filesystem instead of reading from webpacks compiler cache:

https://github.com/TypeStrong/ts-loader/blob/1b84fed767c0fbb41f3da8c8700fc21d69fda3d3/src/watch-run.ts#L60

A workaround for now might be to use transpileOnly and run a seperate checker?

iorate commented 4 years ago

I'm trying to replace readFile with runLoaders when a file has been passed to ts-loader and other loaders are specified after ts-loader.

Files passed to ts-loader can be taken as instance.rootFileNames. Other loaders after ts-loader can be taken as loader.loaders.slice(loader.loaderIndex).

Is this the right way?