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

Watch files onbuild, even if build fails #3081

Merged
merged 20 commits into from Sep 8, 2019

Conversation

mhkeller
Copy link
Contributor

@mhkeller mhkeller commented Aug 24, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Resolves #1773

Description

This PR is a fix for #1773 which created inconsistent behavior between how errors were handled when using rollup watch. Before, if you had a syntax error in your file, the watch process would never get started. Now, it will watch all files in the dependency graph up until the error and keep your watch process running. This avoids the user from having to restart the watch process once the error is fixed and matches the normal rollup watch behavior.

It does this by adding the graph object to the error and then adding the watchers for files already scanned in the graph.

Still needed:

  • Test
  • This breaks some existing tests that rely on the error message format not having a watchFiles array so those need to be fixed.

src/Module.ts Outdated Show resolved Hide resolved
src/watch/index.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #3081 into master will increase coverage by 0.1%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3081     +/-   ##
=========================================
+ Coverage   88.95%   89.06%   +0.1%     
=========================================
  Files         165      165             
  Lines        5721     5713      -8     
  Branches     1742     1738      -4     
=========================================
- Hits         5089     5088      -1     
+ Misses        385      383      -2     
+ Partials      247      242      -5
Impacted Files Coverage Δ
src/Graph.ts 92.94% <ø> (-0.05%) ⬇️
src/watch/index.ts 85.32% <100%> (+3.96%) ⬆️
src/Module.ts 94.25% <100%> (+0.01%) ⬆️
src/ModuleLoader.ts 97.52% <100%> (+0.01%) ⬆️
src/watch/fileWatchers.ts 85.71% <50%> (+2.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1429d57...64180e9. Read the comment docs.

@lukastaegert lukastaegert self-requested a review August 27, 2019 08:49
* Do not add files to Graph.watchFiles after all files have been loaded
  but immediately when starting to load them so that the same watchFile
  logic can be used for successful and unsuccessful builds (and files
  added by plugins are not ignored on errors)
* Do not unwatch files that have been removed if an error occurred but
  only when a build has succeeded
* Do not re-watch files of the previous build when an error occurred as
  those are still being watched; just additionally watch any files
  attached to the error
* Make sure that watching a file automatically adds it to the list of
  "watched" files and only reset this list when clearing unneeded files
* Remove logic to filter for absolute ids for now to restore the old
  behaviour
* Replace some .forEach with for loops for slightly improved performance
given but for any error that occurs during the build phase when there
are already some watchFiles
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Hi, thanks a lot for this PR and sorry if I may have overstepped by adding some changes of my own. Please have a look at the comments and see if everything works as expected. I think this is a great improvement to watch mode that should be released soon!

src/Graph.ts Show resolved Hide resolved
src/Module.ts Show resolved Hide resolved
src/ModuleLoader.ts Show resolved Hide resolved
src/watch/fileWatchers.ts Outdated Show resolved Hide resolved
src/watch/fileWatchers.ts Show resolved Hide resolved
src/watch/index.ts Show resolved Hide resolved
src/watch/index.ts Show resolved Hide resolved
src/watch/index.ts Show resolved Hide resolved
src/watch/index.ts Show resolved Hide resolved
test/watch/index.js Show resolved Hide resolved
@lukastaegert
Copy link
Member

Damn, no idea why the tests are failing on CI while they appear to be running nicely locally, will investigate

@lukastaegert
Copy link
Member

Ok, it seems only chokidar can reliably keep watching deleted files, so I made this a chokidar-only feature. Another good argument for switching to always using chokidar at some point.

@mhkeller
Copy link
Contributor Author

@lukastaegert just tested it on my local project and it works great! Thanks for the thorough look at it!

@oscar-b
Copy link

oscar-b commented Oct 24, 2019

@mhkeller @lukastaegert Using rollup v1.25.1 I'm still seeing the original issue with watch not firing after an initial error. Looking through this PR, it seems like it should be fixed when using chokidar, but it doesn't work here (chokidar v3, macOS).

I'm testing by introducing a syntax error before starting rollup -c -w:

export *a from './Something';

I have this in my rollup.config.js (but also tried with just chokidar: true:

watch: {
	chokidar: { atomic: true },
},

It doesn't make any difference to activate polling mode either using CHOKIDAR_USEPOLLING=true.

Any other ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollup watch has a fatal error on first run if encounters error
4 participants