Improve performance for after-compile plugin.

[Strate]

This fix makes optimization into after-compile plugin, which increases performance of watch mode.
Instead of iterate over all files in instance, iterate only over modified files.

P.S. In my project this fix shave off 1900 ms of each after-compile hook call

[jbrantly]

Hi! Thanks for making a contribution!

The loader actually used to work much like this. The problem is when a change to a file actually causes an error in a completely different file. See for instance https://github.com/TypeStrong/ts-loader/tree/master/test/dependencyErrors

I believe this code would fail in a case like that.

There is quite a bit of discussion on this issue in #78. Ideally the loader/TypeScript needs to calculate a dependency graph for the changed files and only check those.

[Strate]

@jbrantly oh, yes, I've missed this. I'll think about it

[Strate]

@jbrantly I've tried to move pushing file into modifiedFiles hash on every loader call. Seems that it does the job, because webpack actually knows dependencies between a files, because loader actually adds all imported files as dependencies. So, each dependent file passes to the loader and loader pushes it into modifiedFiles.

I've played with this a little, no errors found, all errors shown correctly.

[Strate]

So, at least dependency errors test passed 🔥

[Strate]

I've caught an issue with this solution and fixed it in 8ae7eeb
This issue was related to hide errors when build caused by modified non-ts file.

[Strate]

I have finally implemented reverse dependency graph, which is used to calculate full list of files, which are need to be checked against possible errors.

[athyuttamre]

+1 if this works! A reverse dependency graph can rapidly speed up incremental builds, and would help our large codebase compile quickly.

[johnnyreilly]

Hi @Strate,

Sorry for radio silence.
We've just been doing some fixing up of the tests and are looking at outstanding PRs. Would you be able to resubmit this so we can see the tests go green? We've made some minor changes to the code base but nothing major. Sounds like this could be very handy!

[Strate]

Hi @johnnyreilly ,

I've merged fresh master branch into this one, checks are pending.

[johnnyreilly]

All good on Linux and one failure on windows: declarationWatch

I'll try running this locally and see what I learn. If you have any insights then please do share!

[Strate]

If you have any insights then please do share!

There was one conflict while merge: there was added path.normalize in master branch. I tried to fix it, but maybe I've done something wrong. Maybe that significant for Windows.

[johnnyreilly]

Yes path.normalize is significant for windows. I'm wondering if the same thing needs to be applied to:

importedFiles.forEach(importedFileName => {
                if (!instance.reverseDependencyGraph[importedFileName]) {
                    instance.reverseDependencyGraph[importedFileName] = {}
                }
                instance.reverseDependencyGraph[importedFileName][path.normalize(containingFile)] = true
            })

(Apologies if a bit messy above - I'm doing this on my phone)

[johnnyreilly]

okay - running locally on Windows the declarationWatch test passes: npm run comparison-tests -- --single-test declarationWatch

Running as part of the whole test pack it fails. Go figure.

[johnnyreilly]

So that's totally weird. The declarationWatch test consistently fails on Windows but only in very niche circumstances. So only when run as part of the full test pack (comparison tests and execution tests) but only when comparison tests are run after execution tests. Solution: make comparison tests run before execution tests.

Very odd. Could you do a no-op commit (add a new line or something) so a new build gets queued on AppVeyor please? You may need to merge the master branch into yours once more to get the changed ordering of tests running.

Very very strange behaviour...

[Strate]

Just merged master branch again

[johnnyreilly]

Tests passing! Just out of interest - was there any change in the performance from from your changes after all the different iterations? I know you ended up with something different than when you first submitted the PR.

[Strate]

@johnnyreilly after all iterations rebuild performance became depent on file, which modification trigger rebuilds. For example, if you change file whithout dependants, rebuild will be very fast. If you change the root-most file (or any declaration file), rebuild time became huge. But it is much better when any rebuild time is huge, of course.

[johnnyreilly]

That's great - thanks for all your work @Strate!

@jbrantly - this is a chunky change but looks worth taking. I'm planning to merge unless I hear otherwise from you. I wanted to give you a heads up since you were initially handling this PR. Assuming I do merge I'll plan to squash so we have a single commit. Let me know if you've any issues.

As an aside I'm planning to to a refactor on index.ts at some point and try and modularize it a little bit.

[jbrantly]

I've taken a cursory look at the changes. The overall idea seems right to me: keep track of the dependency graph so that you only get diagnostics on the files that could theoretically have them. Corner cases are potentially a problem, like when the dependency graph itself changes, but if all of the tests are passing that's a very good sign. I can't think of any corner cases that wouldn't work just from looking at the code. Good work!

[jbrantly]

As an aside I'm planning to to a refactor on index.ts at some point and try and modularize it a little bit.

gasp You wouldn't! The whole idea of ts-loader was that it was so simple it could fit into a single file.

Just kidding 😄 I think it probably is time. Idealism ran into hard realities.

[johnnyreilly]

Thanks @jbrantly! I'll plan to merge then.

As another aside - do you have any ideas as to how to put a test in place that could test hot module replacement? I introduced a regression that broke this (fixed with 0.9.4):

However, I can't think of a good way to put a test round it. Essentially you need to get webpack running, touch a file and then ensure that only affected files are re-emitted; not everything. I'm pondering if this could be plugged into the comparison test pack - obviously I could write something hugely custom that could do this but I'd rather re-use the existing pack if it makes sense.

It may be too much of a stretch but I thought it worth asking in case anything occurred to you.

[johnnyreilly]

The whole idea of ts-loader was that it was so simple it could fit into a single file.
Just kidding 😄 I think it probably is time. Idealism ran into hard realities.

Wow! From acorns oak trees do grow 🌳 😄

Yeah - I thought it would be a good way for me to get to know the code; refactoring something forces me to think about it. 🔧 I want to try and make it as easy as possible for people to contribute; to that end I'm going to try and make it as simple as possible. (I'm planning to keep all your comments - there is much value in them!)

[jbrantly]

As another aside - do you have any ideas as to how to put a test in place that could test hot module replacement

I believe (not positive) that looking at "stats" in webpack's watch callback will get you the data you need (it should contain a list of files that it actually changed). I also think the comparison test for that would "just work" since it effectively writes out the stats to a file and compares. I could be wrong though.

[johnnyreilly]

looking at "stats" in webpack's watch callback will get you the data you need

Cool - that's worth bearing in mind.

I also think the comparison test for that would "just work" since it effectively writes out the stats to a file and compares.

There's one thing that I've never understood about the comparison test pack. Patches. What causes them? You know this bit of the docs?

Initial state:

test/someFeature/app.ts
test/someFeature/expectedOutput/bundle.js
test/someFeature/expectedOutput/output.txt

Patch 0

test/someFeature/patch0/app.ts - modified file
test/someFeature/expectedOutput/patch0/bundle.js - bundle after applying patch
test/someFeature/expectedOutput/patch0/output.txt - output after applying patch

What I've never got / comprehended is this: what is modifying the app.ts file? What is causing that to happen such that we get these patches? What is making app.ts change?

I feel I'm missing something very crucial here but I don't know what it is.... I've been up and down the createTest function but feel I'm missing something...

[jbrantly]

You put a modified file in testName/patchN and the test runner will copy that file into the staging at the appropriate time to simulate someone changing the file. webpack picks up the change and re-runs (watch mode) and the whole cycle goes again. testName/expectedOutput/patchN is the expected output after the watch has run. The relevant code is here.

Note that this part is flaky, and I believe I found the cause and solution like 6 months ago deep in the bowels of webpack, but I need to go back and find my work-in-progress on that. And probably everything changed in webpack2.

[johnnyreilly]

You put a modified file in testName/patchN and the test runner will copy that file into the staging at the appropriate time to simulate someone changing the file. webpack picks up the change and re-runs (watch mode) and the whole cycle goes again. testName/expectedOutput/patchN is the expected output after the watch has run. The relevant code is here.

Ahhhhh!!!!!!!!!!!! The penny drops. Yes - I can totally use that I think. I'll have a play. Thanks for the clarification - I'll move what you've just told me into the docs because I completely didn't realised that was what it was for. Better late than never!

Note that this part is flaky, and I believe I found the cause and solution like 6 months ago deep in the bowels of webpack, but I need to go back and find my work-in-progress on that.

That'd be awesome

[johnnyreilly]

Merged - will look to release hopefully towards the end of the week. Once again; thanks @Strate !

[athyuttamre]

Thank you for the merge! This is a huge improvement in day to day workflow for many people.