esm: ensure watch mode restarts after syntax errors #61232
Conversation
Move watch dependency reporting earlier in module resolution to ensure file dependencies are tracked even when parsing fails. Fixes: nodejs#61153
|
Review requested:
|
nodejs-github-bot
added
esm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61232 +/- ##
==========================================
- Coverage 88.54% 88.53% -0.02%
==========================================
Files 704 704
Lines 208734 208760 +26
Branches 40271 40268 -3
==========================================
- Hits 184823 184816 -7
- Misses 15932 15935 +3
- Partials 7979 8009 +30
🚀 New features to boost your workflow:
|
|
Can you add a test? |
|
Just added a test, let me know if that's too broad and shouldn't be tested through cli. |
test/sequential/test-watch-mode.mjs
Outdated
| shouldFail: true, | ||
| }); | ||
|
|
||
| try { |
There was a problem hiding this comment.
why wrapping in try catch? it shouldnt fail
There was a problem hiding this comment.
It's a try finally, not a try catch. It shouldn't fail but wether it does or doesn't, the child processed need to be killed. The same try-finally is used every time restart is used in the file, so I just reused it.
| } | ||
| }); | ||
|
|
||
| it('should watch changes even when there is syntax errors during esm loading', async () => { |
There was a problem hiding this comment.
Can you add the test in a separate file instead of appending it to a test monolith? See https://github.com/nodejs/node/blob/main/doc/contributing/writing-tests.md#how-to-write-a-good-test - a monolithic test increase the chances of flakes in the CI and makes flakes even harder to debug due to the enormous output.
There was a problem hiding this comment.
Totally make sense. Let me know if it's still not the optimal way of doing so.
5 participants
Move watch dependency reporting earlier in module resolution to ensure file dependencies are tracked even when parsing fails.
Fixes: #61153