Fix watch for packages when there are compilation errors in src/#453
Fix watch for packages when there are compilation errors in src/#453harrysarson merged 2 commits intortfeldman:masterfrom
Conversation
| : 'run `elm make` on ' + projectRootDir); | ||
|
|
||
| reject(msg); | ||
| reject(new Error(`\`elm make\` failed with exit code ${exitCode}.`)); |
There was a problem hiding this comment.
The old error was super verbose and not useful when the nice error from elm make is printed just above anyway.
lydell
force-pushed
the
fix-watch-package
branch
from
96b61be to
72bf805
Compare
harrysarson
left a comment
harrysarson
left a comment
There was a problem hiding this comment.
Looking good. I always love to see patches with more lines removed than added.
| projectElmJson = fs.readJsonSync(elmJsonPath); | ||
| } catch (err) { | ||
| console.error('Error reading elm.json: ' + err.message); | ||
| throw process.exit(1); |
There was a problem hiding this comment.
Do we have to crash if we are in watch mode and the project elm json is invalid?
There was a problem hiding this comment.
And what happens if someone edits their elm.json while we are watching tests>
There was a problem hiding this comment.
I didn’t think about that case! This could for sure be a future enhancement.
Currently this is done outside of the watch loop so it would require more changes than I’m comfortable with at the moment. This looks like added code but it’s in fact just moved so were keeping the same behavior as before regarding elm.json at least.
Currently, if elm.json changes while the tests run those changes are just ignored.
There was a problem hiding this comment.
Before this PR generateElmJson read the elm.json file from disk everything. Would that mean that it would detect changes in your elm.json during watch mode?
There was a problem hiding this comment.
Ah, good catch, didn’t think about that change. I’ll have to investigate.
There was a problem hiding this comment.
Turns out Generate.generateElmJson is outside the watch loop too (and was before this PR as well). And moving it into the loop brings more issues:
- We don’t watch
elm.jsonso you need to edit some .elm file to trigger. - If you edit
"source-directories"we need to recalculate which globs the watcher should look at, which is also outside the loop. - Reading elm.json and running
elm-jsoncurrently doesprocess.exit(1)on failure which would need to be refactored. - We basically need to restart the whole thing if elm.json changes.
My conclusion is:
- This PR has not changed behavior.
- Adding support for elm.json changes could be a nice future improvement.
There was a problem hiding this comment.
My conclusion is:
- This PR has not changed behavior.
Good enough for me!
This makes the watcher faster and more stable! As a bonus, this made the code much more organized. - Concentrate all `process.exit` in elm-test.js (except one in Install.js where I found no clean solution). Besides making the code easier to follow, it allows the watcher to continue running (and recover) even if there are errors. - Don’t crash/stop running tests if files are removed. - Pick up new Elm files added. - Fixes #225 We already had code for waiting for the previous run to finish before starting the next run, but it queued up every single file change/add/remove event and ran that many runs one after the other. Now, for all watch events during a run we run _once_ after the current run for all of them. We also sleep 100 ms at the start of each run to catch more events happening close by (such as when adding/removing a whole folder of tests). (Note: We used to sleep at least 500 ms via the [awaitWriteFinish](https://github.com/paulmillr/chokidar#performance) chokidar option.) I’ve also tested manually that running elm-format on save doesn’t cause double test runs: #194 - Fixes #355 We now find the closest elm.json upwards in the folder hierarchy. - Resolves #453 (comment) The watcher now listens for changes to elm.json and properly recalculates things. I thought that I would have to recalculate some things only when elm.json changes for performance, but after making the globbing faster there was no need so we simply rebuild everything on each run. - Closes #463 and closes #464 I can’t find any more places to use async/await. - elm.json is now decoded properly in one place rather than here and there and just being `any`. We now have 0 explicit `any` type annotations! - elm-test.js now only contains CLI logic and subcommand dispatching. The code for running tests have been moved to RunTests.js and to FindTests.js (which used to be called Runner.js). - Improved .flowconfig that is stricter. Closes #478 - And lots of little code improvements along the way! I only touched things that I needed to change anyway in order to not make the diff balloon too much. I’ve done some manual testing on Windows, Mac and Linux to make sure things work like they should.
2 participants
Fixes #399.
Previously, if there was an error in
src/for a package project,elm-test --watchwould immediately exit. This was because we compilesrc/first to get .elmi files for the files in there so thatelmi-to-jsoncan find the exact dependencies there. But since #451 we don’t useelmi-to-jsonto find exact dependencies anymore – we useelm-jsoninstead – so there’s no need for that compilation step anymore. I removed that, which fixes the issue and also improves test run time since the compiler is invoked one time less.As for the
undefinedlog issue, that was because of promises being rejected with strings while the.catchcode expectedErrorobjects."some string".messageis undefined, so that’s where it came from. I updated to reject withErrorobjects instead.This allowed for some pretty nice cleanups.