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

Bail out from watching earlier using watchOptions.ignored #58

Closed
wants to merge 1 commit into from

Conversation

d4rkr00t
Copy link

@d4rkr00t d4rkr00t commented Oct 18, 2017

The Problem:

Contrived example: https://github.com/d4rkr00t/watchpack-infinite-loop-example

In our case, we use monorepo with all of the dependencies in the root node_modules folder which are then symlinked to individual packages, we experience significant performance degradation, because watcher is going very deep through circular symlinks which slows down initial build time, and makes consequent re-builds extremely slow, and the CPU consumption is enormous.

What we've tried:

AFAIK Webpack has 2 ways of ignoring files from watch:

  • watchOptions.ignored – they are passed down to chokidar. This one either not working properly in chokidar or I don't understand how it is supposed to work, because whatever I pass through this option isn't helping at all.
  • WatchIgnorePlugin – that extends NodeWatchFileSystem. And can filter initial set of files, but DirectoryWatcher using setNestedWatching which adds nested directories to watch and that skips WatchIgnorePlugin.

Temporary solution

We came up with a temporary solution that works for us right now, but it is very hacky. We are monkey patching DirectoryWatcher so it ignores node_modules:

const DirectoryWatcher = require('watchpack/lib/DirectoryWatcher');
const _oldcreateNestedWatcher = DirectoryWatcher.prototype.createNestedWatcher;
DirectoryWatcher.prototype.createNestedWatcher = function(dirPath) {
  if (dirPath.includes('node_modules')) return;
  _oldcreateNestedWatcher.call(this, dirPath);
};

That helped a lot, re-builds now are almost instant, comparing to 30s - 60s before this fix. Though, we are not exactly happy about this solution :)

Proposed solution

What I'm proposing in this PR is that we can use watchOptions.ignored to bail out from watching early. Thus breaking out of the circular symlinks hell. Also it speeds up everything a bit, since watchpack doesn't have to care about ignored folders/files.

Maybe you can suggest better approach for doing this. Or better places where I can add this check.

Also I haven't added any tests yet, since I didn't want to spend time writing them being not sure whether proposed approach is ok for you or not. But if it's fine with you, I'll add tests.

I looked at this PR #41 which potentially can solve this issue, but since it has probably died almost a year ago I think considering other options make total sense.

@jsf-clabot
Copy link

jsf-clabot commented Oct 18, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@codecov
Copy link

codecov bot commented Oct 18, 2017

Codecov Report

Merging #58 into master will decrease coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #58     +/-   ##
=========================================
- Coverage   95.67%   95.18%   -0.5%     
=========================================
  Files           3        3             
  Lines         347      353      +6     
  Branches       90       94      +4     
=========================================
+ Hits          332      336      +4     
- Misses         15       17      +2
Impacted Files Coverage Δ
lib/DirectoryWatcher.js 94.93% <100%> (-0.74%) ⬇️

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 9fc9be0...a85cd85. Read the comment docs.

@sokra
Copy link
Member

sokra commented Jul 10, 2019

fixed

@sokra sokra closed this Jul 10, 2019
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.

3 participants