Uh oh!
There was an error while loading. Please reload this page.
feat: add depending .d.ts files as changed when watch is triggered - #698
Conversation
If a project depends on custom definition files, meaning not the ones in node_modules, then it should consider them changed when webpack triggers a watch-run. Since webpack doesn't know about definition files, we use the one detected by the languageService which should be more accurate anyway. ClosesTypeStrong#697
johnnyreilly
commented
Dec 19, 2017
Thanks @KnisterPeter - I'm pretty snowed right now but I will get back to you with a review once things ease off. |
KnisterPeter
commented
Dec 28, 2017
@johnnyreilly any update on this? |
johnnyreilly
commented
Dec 28, 2017
Hi @KnisterPeter, Thanks for the PR - it hasn't been forgotten. I'm taking some time out over Christmas and I'm planning to take a look at it once #685 has been merged. Typically we have a much faster turnaround on PRs but on this occasion there is going to be a lag as 685 is so significant. Essentially I'm trying to avoid merge hell 😉 Please bear with me - I will get to this PR as soon as I can. Thanks again for contributing - I really appreciate it. |
KnisterPeter
commented
Jan 24, 2018
I've just resolved the conflicts. Before merging I'll test if the new watcher code is enough without my PR. |
KnisterPeter
commented
Jan 24, 2018
After testing with the current master, I have to say that my PR is still needed to rebuild when only declaration files change. |
johnnyreilly
commented
Jan 24, 2018
Cheers @KnisterPeter |
johnnyreilly
commented
Jan 24, 2018
You tested with |
KnisterPeter
commented
Jan 24, 2018
A sorry, I missed that. I'll test again 😄 |
@johnnyreilly Nope, even with that its still missing files (or at least not reporting errors). |
johnnyreilly
commented
Jan 24, 2018
👍 |
johnnyreilly
commented
Jan 25, 2018
Hey @KnisterPeter, It looks like the declarationWatch test is broken by this (which may make sense given the nature of the change). Can you advise on how you would expect this test case to behave post your change? Test in question here: https://github.com/TypeStrong/ts-loader/tree/master/test/comparison-tests/declarationWatch Explanation of our comparison test pack here: https://github.com/TypeStrong/ts-loader/blob/master/test/comparison-tests/README.md Happy to help if there's confusion. |
KnisterPeter
commented
Jan 25, 2018
@johnnyreilly I could try to fix the test the next days. This is for example helpful if you have a project with yarn workspaces, lerna or other cases (like in the example repo created by me). |
johnnyreilly
commented
Jan 25, 2018
Ok - hopefully you should be able to just regenerate the output of the test. Assuming it looks sane then we're good from a functional perspective. Question: do you know what sort of impact this change has on performance? |
@johnnyreilly Thats a bit strange. When running locally with Oh, i see, its just the TSC 2.7-rc builds. But event stranger I did run them with 2.7-rc locally as well. |
KnisterPeter
commented
Jan 28, 2018
Regarding the performance it should have no or low impact. This change just add watchers for the included definition files. |
KnisterPeter
commented
Jan 30, 2018
@johnnyreilly Any idea? |
johnnyreilly
commented
Jan 30, 2018
Sorry about the delay in responding @KnisterPeter - I've been very occupied with webpack 4 work. Could you delete the 2.7 expected output for the failing test and then regenerate it using |
KnisterPeter
commented
Jan 30, 2018
@johnnyreilly Done. Thumbs pressed 😃 |
KnisterPeter
commented
Jan 30, 2018
@johnnyreilly All done. Thanks for your help. |
johnnyreilly
commented
Jan 30, 2018
No bother. Question: how keen are you for this to use this with webpack 3? I'm planning to release ts-loader 4 with webpack 4 support. It'd make my life easier if I included this with that. However if you're unlikely to be able to move to webpack 4 soon after release then let me know. It's probably still doable to ship another ts-loader on the v3 codebase |
KnisterPeter
commented
Jan 30, 2018
I would be very glad if it will be merged for webpack 3. I'm not sure how fast we could move on to webpack 4. |
johnnyreilly
commented
Jan 30, 2018
OK - I'll see what I can do. |
johnnyreilly
commented
Jan 30, 2018
Released as https://github.com/TypeStrong/ts-loader/releases/tag/v3.4.0 - thanks for your help! |
KnisterPeter
commented
Jan 30, 2018
Wow. Thank you 👍 |
If a project depends on custom definition files, meaning not the ones in node_modules, then it should consider them changed when webpack triggers a watch-run.
Since webpack doesn't know about definition files, we use the one detected by the languageService which should be more accurate anyway.
Closes#697