Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve watcher #475

Merged
merged 67 commits into from
Dec 17, 2020
Merged

Improve watcher #475

merged 67 commits into from
Dec 17, 2020

Conversation

lydell
Copy link
Collaborator

@lydell lydell commented Nov 21, 2020

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 Watcher: Cancel previous compilation and test runs #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 chokidar option.)

    I’ve also tested manually that running elm-format on save doesn’t cause double test runs: Cancel previous running compile process in watch mode #194

  • Fixes Error if run from a subdirectory #355

    We now find the closest elm.json upwards in the folder hierarchy.

  • Resolves Fix watch for packages when there are compilation errors in src/ #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 Replace new Promise and .then with async/await #463 and closes Moving to async/await: lib/Runner.js #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 Ban the flow any type. #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.

lib/Compile.js Show resolved Hide resolved
lib/FindTests.js Show resolved Hide resolved
lib/FindTests.js Show resolved Hide resolved
lib/FindTests.js Show resolved Hide resolved
lib/FindTests.js Show resolved Hide resolved
lib/Parser.js Show resolved Hide resolved
lib/RunTests.js Show resolved Hide resolved
lib/RunTests.js Show resolved Hide resolved
lib/RunTests.js Show resolved Hide resolved
lib/Supervisor.js Show resolved Hide resolved
@harrysarson
Copy link
Collaborator

Thanks for the comments explaining the rearrangements; they are really helpful. I am getting there:

image


Thanks @pipefail too for your help, much appreciated :)

As a bonus, functions are now either pure or side-effect-only without
return values. It is very clear now how the compilation steps work.
lib/Generate.js Show resolved Hide resolved
lib/Generate.js Outdated Show resolved Hide resolved
lib/Generate.js Outdated Show resolved Hide resolved
lib/Install.js Outdated Show resolved Hide resolved
lib/elm-test.js Outdated Show resolved Hide resolved
lib/elm-test.js Outdated Show resolved Hide resolved
@harrysarson
Copy link
Collaborator

Just got runTests to go. Nearly there 🎉

Copy link
Collaborator

@harrysarson harrysarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy!

@harrysarson
Copy link
Collaborator

I like the queuing a lot and thanks for the help finding which code had moved where. You ready for me to merge?

@harrysarson harrysarson self-assigned this Dec 17, 2020
@lydell
Copy link
Collaborator Author

lydell commented Dec 17, 2020

I’m ready! :shipit:

@harrysarson harrysarson merged commit 45c5245 into rtfeldman:master Dec 17, 2020
@lydell lydell deleted the improve-watcher branch December 17, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for review Pull request needs a review
Projects
None yet
3 participants