fix #433, ensure deps not cleared for HMR

[HerringtonDarkholme]

Discussed in the original issue.

[johnnyreilly]

I guess you're looking at the tests?

[HerringtonDarkholme]

Yes, I wonder why I cannot execute comparison test on my local machine. Turns out to be wrong TS version. Working on it.

[johnnyreilly]

Cool

[HerringtonDarkholme]

My initial observation: clearDependencies and addDependency do force refresh webpack rebuild. This is in line with my guess.

All test cases fails with at least one patch, which stands for a file change. Yet not all patches fail.

The tricky part is: not all change effects compiled output. For example, changing var a: any = 123 to var a: number = 123 will not trigger webpack output because the output file does not change! So the test cases fail.

On the other hand, test like declarationWatch will break because the compiled js does not include declaration, so webpack will not pick change up in watch mode.

I'd rather say it is a trade-off. Forcing rebuild will ensure type/declaration errors up to date. But it also introduces more compiling overhead and breaks HMR.

[jbrantly]

It's hard to me to comment too much on the HMR side of things without really digging in deep, but I would say that keeping functionality like declarationWatch is pretty important. It's totally possible there is a bug somewhere in ts-loader that's breaking HMR, but ideally a fix would both resolve HMR and keep declarationWatch functionality.

You're saying that clearDependencies is breaking HMR. clearDependencies is really only there for the case where dependencies are removed from the tree, which is probably somewhat rare. I'd be interested in seeing what only using addDependencies would result in (so you'd still get declarationWatch functionality, at least for additions and changes).

It's also possible the API changed for webpack2 and you no longer need to clearDependencies. And lastly it's always possible there is a bug in webpack where clearing dependencies triggers this behavior when it really shouldn't.

At the end of the day though, we need to call addDependencies for declaration files. That's the whole point of that API being included in webpack, to declare additional dependencies for the current file that webpack itself isn't aware of.

[HerringtonDarkholme]

But sole addDependency also breaks HMR. addDependency has the same effect of importing a new file. And webpack gives up HMR when new files are imported. ( tested by editing local ts-loader, only removing clearDependencies also breaks HMR)

So to support both dependency watch and HMR would require managing dependencies in ts-loader (which we already have done) and adding declaration file to watch list conditionally (don't know how).

[HerringtonDarkholme]

Pushed further fix. The error diff is trivial. Maybe we need a baseline bump.

[johnnyreilly]

The error diff is trivial. Maybe we need a baseline bump.

Do you mean the changes in the comparison tests are reasonable and don't represent a regression in functionality? If so, do you want to regenerate the necessary test results?

[HerringtonDarkholme]

Still having two errors remaining... investigating....

[HerringtonDarkholme]

Remaining failure is due to how ts-loader produces errors.

Because changing interface/ type declaration does not change output, so no webpack output is produced. However, every ts-loader error is bound to some module. If not emission is done after compilation, webpack will not produce error.

For comparison, awesome-ts-loader does not rely on compilation to report error.

[johnnyreilly]

Would it be possible to switch to that behaviour in ts-loader? I'm keen not to lose error reporting but I don't mind so much how it happens. cc @jbrantly @s-panferov - all opinions / help welcomed!

[johnnyreilly]

Not going well?

[HerringtonDarkholme]

Test is too fragile. An output relies on stats and error report also relies on stats. Output will not be emitted for stats with same hash but error reports need to be emitted every time. Output with the same hash seems undefined in webpack.

This conflict makes output annoyingly fickle. Every test run will produce different failure.

[johnnyreilly]

Frustrating...

[johnnyreilly]

Would it be worth having the new functionality behind a hmrMode flag and falling back to existing functionality if it's not set? Or is that a bad idea?

[HerringtonDarkholme]

I have already done that. But it has too much breaking in tests.