Don't include appended TS extension in webpack dependencies. #497

WearyMonkey · 2017-03-10T07:55:16Z

ts-loader incorrectly includes the appended '.ts' suffix of files matching the appendTsSuffixTo option in the dependency path, e.g. addDependency('styles.css.ts').

As webpack is unable to find these files on the filesystem it appears to mark the module as dirty when ANY file in the entry point's dependency tree changes. The end result is any change will cause all modules to be re-compiled.

As the bug affects watch mode, I could not find a way to verify the fix in the current test infrastructure.

I did however create a sample project here:
https://github.com/WearyMonkey/ts-loader-dependencies-bug

fixes #496

johnnyreilly · 2017-03-10T08:12:04Z

Thanks for the PR - we really appreciate it! Just waiting to see if the tests pass..

johnnyreilly · 2017-03-10T08:51:45Z

Think the failure might just be flakiness. Have restarted the associated test pack

johnnyreilly · 2017-03-10T09:14:40Z

Okay we're good to merge insofor that existing functionality is not impacted.

My 1 reservation is we have no test that will prevent regression in future. My feeling is that a comparison test would cover this. Have a read of the readme under comparison tests for a proper run through of how these work.

Essentially these tests use watch mode. The presence of a patch0 / patch1 etc directory allows for the simulation of updating a file during watch mode. The updates are applied on a 5 secondly basis. This is exactly what you want to test.

The output that would result (specifically output.txt after the file update) should indicate that, with your PR, less files are being recompiled. You should be able to repurpose your example repo into a comparison test and I'll be happy to advise. BTW, we only need to generate test output for the latest version of TypeScript; 2.2.1.

If you'd like an example of what a test that works like this looks like then take this one: https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/aliasResolution

Notice https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/aliasResolution/patch0/common/components which is applied 5 secs after initial compilation.

Notice https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/aliasResolution/expectedOutput-2.2 which is the output

Notice https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/aliasResolution/expectedOutput-2.2/output.txt which is the output.txt that I was speaking about.

Any questions then shout - I'm aware it's a little complicated. It's the price we pay for preventing regressions...

WearyMonkey · 2017-03-10T11:48:29Z

I had a go at writing a comparison test. 'Output.txt is generated from the stats output which details all bundles, not just the ones built or individual resources loaded. Any ideas would be appreciated!

…

On Fri, 10 Mar 2017 at 6:14 pm, John Reilly ***@***.***> wrote: Okay we're good to merge in that existing functionality is not impacted. My 1 reservation is we have no test that will prevent regression in future. My feeling is that a comparison test would cover this. Have a read of the readme under comparison tests for a proper run through. Essentially these tests use watch mode. The presence of a patch0 directory allows for the simulation of updating a file during watch mode. This is exactly what you want to test. The output that would result (specifically output.txt after the file update) should indicate that, with your PR, less files are being recompiled. You should be able to repurpose your example repo into a comparison test and I'll be happy to advise. BTW, we only need to generate test output for the latest version of TypeScript; 2.2.1. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8NUZAYgs1fFjc-pvheXCdlgz-zRlw7ks5rkRSBgaJpZM4MZE_A> .

johnnyreilly · 2017-03-10T11:56:16Z

I had a go at writing a comparison test. Output.txt is generated from the stats output which details all bundles, not just the ones built or individual resources loaded.

I think that's okay though isn't it? Isn't the important bit the [built] bit that appears at the end of the module line?

I think a good illustration of this is the issue372 (shocking name) test. Here's the initial output:

    Asset     Size  Chunks             Chunk Names
bundle.js  3.02 kB       0  [emitted]  main
chunk    {0} bundle.js (main) 221 bytes [entry] [rendered]
    [0] ./.test/issue372/foo.ts 90 bytes {0} [built]
    [1] ./.test/issue372/~/a/index.js 21 bytes {0} [built]
    [2] ./.test/issue372/app.ts 110 bytes {0} [built]

Here's the patch0 output :

    Asset     Size  Chunks             Chunk Names
bundle.js  3.03 kB       0  [emitted]  main
chunk    {0} bundle.js (main) 228 bytes [entry] [rendered]
    [0] ./.test/issue372/foo.ts 90 bytes {0}
    [1] ./.test/issue372/~/a/index.js 21 bytes {0}
    [2] ./.test/issue372/app.ts 117 bytes {0} [built]

So the verification that your change works correctly is the number of modules that are listed with built in the test output for patch0?

WearyMonkey · 2017-03-10T11:59:36Z

Cool thanks I'll take a look.

WearyMonkey · 2017-03-10T12:03:15Z

BTW I'm curious why the loader needs to manually add all dependencies and not just declaration files?
Webpack would detect dependencies from the transpiled import statements would it not?
Cheers!

johnnyreilly · 2017-03-10T12:10:01Z

Webpack would detect dependencies from the transpiled import statements would it not?

I didn't write that so I'm not sure. I'd speculate it's because webpack receives the JavaScript not the TypeScript. So if there's an import statement for an interface then webpack won't know about it unless you tell it explicitly. But as I say; I didn't write that so I'm not totally sure.

jbrantly · 2017-03-10T12:47:12Z

BTW I'm curious why the loader needs to manually add all dependencies and not just declaration files?
Webpack would detect dependencies from the transpiled import statements would it not?

It was because Webpack had no way to remove dependencies other than clearing them which also messed with dependencies that webpack itself added. So the loader had to take over adding those dependencies back as well. At least that's my recollection. That was like 2 years ago though so things could have changed since then, esp. with the release of webpack 2.

The reason that you need to clear (instead of just being additive) is that it's possible for the source to remove dependencies.

WearyMonkey · 2017-03-11T09:04:13Z

@jbrantly thanks for the info!

is that it's possible for the source to remove dependencies.

What exactly does that mean?

If typescript is removing dependencies, wouldn't the import statement be missing from the transpiled JS and therefore not be picked up by webpack?

Cheers!

johnnyreilly · 2017-03-13T06:35:37Z

I guess the test is still a work in progress?

johnnyreilly · 2017-03-13T07:43:54Z

Also, didn't you want to use the patch approach to test dependencies are built in the manner you hope?

HerringtonDarkholme · 2017-03-13T08:16:37Z

@WearyMonkey See #471 for more details on clearDependency/addDependency.

Fairly, what needed here is just adding declaration file to watch list and re-checking the whole program once a watchee is changed. The same approach is adopted by atl.

johnnyreilly · 2017-03-13T11:15:06Z

If you're having problems then feel free to post the issues you're experiencing. I'm happy to be on hand to provide advice as you work on this.

WearyMonkey · 2017-03-13T11:21:59Z

Cheers! I'll take another look at this tonight.

…

On Mon, 13 Mar 2017 at 8:15 pm, John Reilly ***@***.***> wrote: If you're having problems then feel free to post the issues you're experiencing. I'm happy to be on hand to provide advice as you work on this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8NUU9mKaGsXTekMijuP2JTiVk29WSRks5rlSU7gaJpZM4MZE_A> .

HerringtonDarkholme · 2017-03-13T12:08:21Z

The difficulty I encounter is non deterministic stats. I cannot even make test pass on the master branch.
And removing addDep/clearDep only make stochastic tests crazier.
However, I really don't have any idea to fix it. Maybe our test can be less strict.

johnnyreilly · 2017-03-13T12:18:18Z

Maybe our test can be less strict.

Yeah - I'm open to this. I inherited the the comparison test pack; the grunt work was done by @jbrantly. It has become progressively less strict in the time that I've been looking after ts-loader. Take a look at the functions starting here for examples of the sorts of differences in output that the pack now allows. If it makes sense to take this further than that's totally on the table.

WearyMonkey · 2017-03-14T16:55:14Z

@johnnyreilly the latest test failures are due to getTimes() returning a map with all files, not just changed on files. This causes the versions of non-changed files to be incremented. Not sure why this is happening on my machine but not the CI machines.

WearyMonkey · 2017-03-14T16:56:43Z

It could also explain some the of the flakiness in other tests.

johnnyreilly · 2017-03-14T17:10:06Z

@johnnyreilly the latest test failures are due to getTimes() returning a map with all files, not just changed on files. This causes the versions of non-changed files to be incremented.

Actually that's an interesting point. @jbrantly - would you be able to advise on this?

        Object.keys(watcher.getTimes())
            .filter(filePath => !!filePath.match(constants.tsTsxJsJsxRegex))
            .forEach(filePath => {
                filePath = path.normalize(filePath);
                const file = instance.files[filePath];
                if (file) {
                    // we are not checking for differences here; just incrementing regardless
                    file.text = utils.readFile(filePath) || '';
                    file.version++;
                    instance.version++;
                    instance.modifiedFiles[filePath] = file;
                }
            });

Is there any reason that we always increment versions here? There's no test to see if there is a difference from the previous version which would seem like a reasonable thing to do. Can you recall why this was?

I'm guessing the implicit assumption is that if a file is supplied then it has changed. Hence this implementation; but I don't actually know. If you can recall that would be handy info.

johnnyreilly · 2017-03-14T17:14:54Z

FWIW we didn't use getTimes() until v2.0.1. We previously used mtimes which was not part of the official API. mtimes and getTimes() were not one and the same thing. If getTimes() produces a full map of files and mtimes was just the changed ones then that's actually a subtle difference that we may want to cater for...

WearyMonkey · 2017-03-14T17:20:15Z

@johnnyreilly mtimes was re-added in watchpack 1.3.1.
I tried changing back to mtimes, but got the same result.

What did work for me was manually tracking timestamps for changes, e.g.:

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

johnnyreilly · 2017-03-14T17:30:22Z

@johnnyreilly mtimes was re-added in watchpack 1.3.1.

You're quite right; it was and there's a story behind it 😉 Essentially it was re-added as deprecated to avoid breaking ts-loader / atl etc. We can't rely upon it and need to stick with getTimes().

What did work for me was manually tracking timestamps for changes...

I rather suspect we should switch to use this approach in place of what we have at present.

WearyMonkey · 2017-03-14T17:33:35Z

I rather suspect we should switch to use this approach in place of what we have at present.

Well here's my PR: #500

:)

johnnyreilly · 2017-03-14T17:40:39Z

Thanks! I've commented there. Just going offline for now...

johnnyreilly · 2017-03-16T21:32:49Z

Just merged #500 and published v2.0.2 - just realised I'd forgotten all about the original PR!

WearyMonkey · 2017-03-20T02:01:00Z

@johnnyreilly sorry for being unresponsive, have been travelling.

Is there a way to trigger a CI build of this branch off the new master?

johnnyreilly · 2017-03-20T16:23:12Z

I've just retriggered a build. I think that runs off the new master....

WearyMonkey · 2017-03-22T06:15:06Z

@johnnyreilly so I guess this is good to merge?

johnnyreilly · 2017-03-22T07:05:44Z

Yes - it will be. I've got a lot going on right now for my wife's birthday. But yes it will be!

WearyMonkey · 2017-03-22T07:08:32Z

No worries :) happy b'day to your wife!

Don't include appended TS extension in dependencies.

4b52c30

test

62cfe1f

fix test

6bd3431

.

bb3ee50

johnnyreilly mentioned this pull request Mar 14, 2017

Slow rebuild times for ts-loader@2.0.1 #499

Closed

.

fe21e58

johnnyreilly merged commit 5e20c2c into TypeStrong:master Mar 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't include appended TS extension in webpack dependencies. #497

Don't include appended TS extension in webpack dependencies. #497

WearyMonkey commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017 •

edited

Loading

WearyMonkey commented Mar 10, 2017 via email

johnnyreilly commented Mar 10, 2017 •

edited

Loading

WearyMonkey commented Mar 10, 2017

WearyMonkey commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017

jbrantly commented Mar 10, 2017

WearyMonkey commented Mar 11, 2017

johnnyreilly commented Mar 13, 2017

johnnyreilly commented Mar 13, 2017

HerringtonDarkholme commented Mar 13, 2017

johnnyreilly commented Mar 13, 2017

WearyMonkey commented Mar 13, 2017 via email

HerringtonDarkholme commented Mar 13, 2017

johnnyreilly commented Mar 13, 2017

WearyMonkey commented Mar 14, 2017

WearyMonkey commented Mar 14, 2017

johnnyreilly commented Mar 14, 2017 •

edited

Loading

johnnyreilly commented Mar 14, 2017

WearyMonkey commented Mar 14, 2017 •

edited

Loading

johnnyreilly commented Mar 14, 2017

WearyMonkey commented Mar 14, 2017

johnnyreilly commented Mar 14, 2017

johnnyreilly commented Mar 16, 2017

WearyMonkey commented Mar 20, 2017

johnnyreilly commented Mar 20, 2017

WearyMonkey commented Mar 22, 2017

johnnyreilly commented Mar 22, 2017

WearyMonkey commented Mar 22, 2017

Don't include appended TS extension in webpack dependencies. #497

Don't include appended TS extension in webpack dependencies. #497

Conversation

WearyMonkey commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017 • edited Loading

WearyMonkey commented Mar 10, 2017 via email

johnnyreilly commented Mar 10, 2017 • edited Loading

WearyMonkey commented Mar 10, 2017

WearyMonkey commented Mar 10, 2017

johnnyreilly commented Mar 10, 2017

jbrantly commented Mar 10, 2017

WearyMonkey commented Mar 11, 2017

johnnyreilly commented Mar 13, 2017

johnnyreilly commented Mar 13, 2017

HerringtonDarkholme commented Mar 13, 2017

johnnyreilly commented Mar 13, 2017

WearyMonkey commented Mar 13, 2017 via email

HerringtonDarkholme commented Mar 13, 2017

johnnyreilly commented Mar 13, 2017

WearyMonkey commented Mar 14, 2017

WearyMonkey commented Mar 14, 2017

johnnyreilly commented Mar 14, 2017 • edited Loading

johnnyreilly commented Mar 14, 2017

WearyMonkey commented Mar 14, 2017 • edited Loading

johnnyreilly commented Mar 14, 2017

WearyMonkey commented Mar 14, 2017

johnnyreilly commented Mar 14, 2017

johnnyreilly commented Mar 16, 2017

WearyMonkey commented Mar 20, 2017

johnnyreilly commented Mar 20, 2017

WearyMonkey commented Mar 22, 2017

johnnyreilly commented Mar 22, 2017

WearyMonkey commented Mar 22, 2017

johnnyreilly commented Mar 10, 2017 •

edited

Loading

johnnyreilly commented Mar 10, 2017 •

edited

Loading

johnnyreilly commented Mar 14, 2017 •

edited

Loading

WearyMonkey commented Mar 14, 2017 •

edited

Loading