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

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

Closed
iorate opened this issue May 17, 2020 · 6 comments · Fixed by #1115
Closed

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

iorate opened this issue May 17, 2020 · 6 comments · Fixed by #1115

Comments

@iorate
Copy link
Contributor

iorate commented May 17, 2020

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

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

iorate commented May 18, 2020

@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 (cc31287).
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
Copy link
Member

johnnyreilly commented May 18, 2020

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

iorate commented May 18, 2020

Sorry, I was mistaken.
The important point of the commit (cc31287) 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 44a2cdb :

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

The commit 388f2a6 , 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, cc31287 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
Copy link
Contributor

Zn4rK commented May 18, 2020

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:

nFilePath => readFile(nFilePath) || ''

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

@iorate
Copy link
Contributor Author

iorate commented May 19, 2020

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?

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants