fix: watch-run#1083
Conversation
|
Thanks for contributing! I must admit to wondering what the consequence of removing the regex would be... I have a feeling I put that in for a reason but, shame on me, I didn't put a comment to explain 😭 |
|
@johnnyreilly Yeah, I tried a bunch of stuff to try and figure out why it was there to begin with. // many more of these, on every single watchRun
// since the first check doesn't include equality, they are updated on every "watch-run".
filePath node_modules/typescript/lib/README.md date: 499162500010
filePath node_modules/typescript/lib/diagnosticMessages.generated.json, date: 499162500010
filePath node_modules/typescript/lib/typesMap.json date: 499162500010But never the file that triggered the change - it eventually gets updated by the successLoader, but that's after it's needed - i.e. after any transformations. Having it this way, the files are never updated in the cache any more times than necessary. It could potentially make it quicker. |
|
Cool - thanks for the explanation. We've got some big stuff happening with project references right now; see this PR: #1076 I'd like to get that landed and then take a look at this afterwards. Please bear with us! |
|
Alright. Sounds good! I'll just use my patch for now. And FYI I noticed that my excerpt above only included files from There's another conditional further down that I think can be removed completely (or at least modified) as well. The typescript program needs to have a new version of the file that's been changed before emitting basically. |
|
v7.0.0 is now merged - I think you may have some merge conflicts I'm afraid! |
|
I've rebased and updated, so I guess we're only waiting on Appveyor and Travis! :) |
|
Heya! I'll admit to some nervousness around the change to this test output here: Is this what you meant when you said:
I am aware of some quirkiness in our comparison tests which @sheetalkamat bumped on here: #1076 (comment) That change was rolled back later on. So this may be fine... Thoughts? |
I understand your nervousness and I'm a bit nervous too! I'm fairly confident that It should work as expected though. That's basically what I meant. I just tested this manually, and ideally this file should be reverted to emit an error - since it still does that (as it should).
We could maybe make the test flaky instead and take another look at improving the comparison tests in the future? |
|
Let's roll with it as is for now. Worst comes to worst we can revert this later, but I'll go 🤞 it's down to the quirkiness of the comparison test pack. Could you update the |
Fair enough! :) I guess we just have to remember that it actually should emit an error. But the correct expected output still exists in
Done! 👍Thank you for taking the time with this! |
Bump minimum node version to 10
| bundle.js 4.31 KiB main [emitted] main | ||
| ../app.d.ts 11 bytes [emitted] | ||
| Entrypoint main = bundle.js | ||
| [./app.ts] 186 bytes {main} [built] [1 error] |
There was a problem hiding this comment.
How is this correct change.. Is this issue with the test (where it baselines wrong result) or the change?
There was a problem hiding this comment.
I believe that's an issue with this test. It should (and still does) produce the expected error. We could revert this make it flaky instead?
There was a problem hiding this comment.
I've manually verified projectReferencesWatch_Composite_WatchApi and projectReferencesWatch_WatchApi, both emit the error when running "manually" but not when using the comparison tests.
There was a problem hiding this comment.
Is this issue with the test (where it baselines wrong result) or the change?
I think the problem may be the comparison test pack which seems to be demonstrating unhelpful (incorrect) behaviour. The comparison test pack is not as reliable as I'd like (and I'm partly responsible).
This doesn't give me a warm fluffy feeling but I don't believe it represents a bug in ts-loader.
| bundle.js 4.38 KiB main [emitted] main | ||
| bundle.js 4.31 KiB main [emitted] main | ||
| Entrypoint main = bundle.js | ||
| [./app.ts] 186 bytes {main} [built] [1 error] |
* fix: watch-run * Update package.json Bump minimum node version to 10 Co-authored-by: John Reilly <johnny_reilly@hotmail.com>
3 participants
Closes #1082
I need a bit of help verifying that these tests are correct. I tried to understand what's happening with _WatchApi, but couldn't wrap my head around it.
It looks scary to change
/projectReferencesWatch_Composite_WatchApi/expectedOutput-3.8/patch4/output.txtto not emit an error, butprojectReferencesWatchstill has the corresponding error.