fix: watch-run

[Zn4rK]

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.txt to not emit an error, but projectReferencesWatch still has the corresponding error.

[johnnyreilly]

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 😭

[Zn4rK]

@johnnyreilly Yeah, I tried a bunch of stuff to try and figure out why it was there to begin with.
What essentially is let through the conditionals are files like these:

// 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: 499162500010

But 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.

[johnnyreilly]

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!

[Zn4rK]

Alright. Sounds good! I'll just use my patch for now.

And FYI I noticed that my excerpt above only included files from typescript, but the list contains a whole bunch of files from many different packages.

There's another conditional further down that I think can be removed completely (or at least modified) as well.
If the file has changed since the last time it was included (i.e. since the initial build), my "fix" would include it.

The typescript program needs to have a new version of the file that's been changed before emitting basically.

[johnnyreilly]

v7.0.0 is now merged - I think you may have some merge conflicts I'm afraid!

[Zn4rK]

I've rebased and updated, so I guess we're only waiting on Appveyor and Travis! :)

[johnnyreilly]

Heya!

I'll admit to some nervousness around the change to this test output here:

/~https://github.com/Zn4rK/ts-loader/blob/82c192229be94edec5286e10698fa18230d33eb7/test/comparison-tests/projectReferencesWatch_WatchApi/expectedOutput-3.8/patch4/output.txt

Is this what you meant when you said:

It looks scary to change /projectReferencesWatch_Composite_WatchApi/expectedOutput-3.8/patch4/output.txt to not emit an error, but projectReferencesWatch still has the corresponding error.

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?

[Zn4rK]

Heya!

I'll admit to some nervousness around the change to this test output here:

/~https://github.com/Zn4rK/ts-loader/blob/82c192229be94edec5286e10698fa18230d33eb7/test/comparison-tests/projectReferencesWatch_WatchApi/expectedOutput-3.8/patch4/output.txt

Is this what you meant when you said [...]

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).

(I made sure to include watch: true in the config)

I am aware of some quirkiness in our comparison tests which @sheetalkamat bumped on here: #1076 (comment)

We could maybe make the test flaky instead and take another look at improving the comparison tests in the future?

[johnnyreilly]

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 package.json to 7.0.1 please? And add an entry to CHANGELOG.md - don't forget to thank yourself!

[Zn4rK]

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.

Fair enough! :) I guess we just have to remember that it actually should emit an error. But the correct expected output still exists in projectReferencesWatch.

Could you update the package.json to 7.0.1 please? And add an entry to CHANGELOG.md - don't forget to thank yourself!

Done! 👍Thank you for taking the time with this!

[johnnyreilly]

Released! /~https://github.com/TypeStrong/ts-loader/releases/tag/v7.0.1

[sheetalkamat]

How is this correct change.. Is this issue with the test (where it baselines wrong result) or the change?

[Zn4rK]

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?

[Zn4rK]

I've manually verified projectReferencesWatch_Composite_WatchApi and projectReferencesWatch_WatchApi, both emit the error when running "manually" but not when using the comparison tests.

[johnnyreilly]

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.

[sheetalkamat]

Same here?

[Zn4rK]

Same as above.