Skip to content

Add/ignoring handler#202

Closed
retrofox wants to merge 3 commits intobrowserify:masterfrom
retrofox:add/ignoring-handler
Closed

Add/ignoring handler#202
retrofox wants to merge 3 commits intobrowserify:masterfrom
retrofox:add/ignoring-handler

Conversation

@retrofox
Copy link
Copy Markdown

This PR avoids create chokidar watcher instances when the file matchs with the ignoring rule defined through --ignore-watch parameter so the idea try to is optimize resources.

@retrofox
Copy link
Copy Markdown
Author

In my app when I run with current 3.1.0 version I get the follow:

1610040 bytes written to > "public/build.js" (7.52 seconds)
1610040 bytes written to > "public/build.js" (8.21 seconds)
1610040 bytes written to > "public/build.js" (8.65 seconds)
1610040 bytes written to > "public/build.js" (15.62 seconds)
1610040 bytes written to > "public/build.js" (5.70 seconds)

With this PR:

1610284 bytes written to > "public/build.js" (7.34 seconds)
1610284 bytes written to > "public/build.js" (0.64 seconds)
1610284 bytes written to > "public/build.js" (0.67 seconds)
1610284 bytes written to > "public/build.js" (0.65 seconds)
1610284 bytes written to > "public/build.js" (0.59 seconds)

I'm passing `--ignore-watch="/node_modules/" to watchify

@zertosh
Copy link
Copy Markdown
Member

zertosh commented Apr 14, 2015

@retrofox My comment in the last paragraph of #193 (comment) still stands. Can you implement it in the same way as browserify does?

Those sample stats look way off for a build that size. There is no way you're bundling 10k files in 1.6mb. Did you try what I suggested in #193 (comment)? The bit about removing the . (period)? Trying this command watchify index.js --ignore-watch -o public/build.js? Please try that and run test. What happens?

@retrofox
Copy link
Copy Markdown
Author

@zertosh Hi dude. Thanks for your answer.
Well ... I can't define only index.js. I don't why but I lose to keep watching some files. I guess it's because we have a lot of files in our structure and use only one file isn't enough to grab to all of them.
Anyway this PR is independent of this results. Let me understand clearly what do you mean with browserify mode.
Thank you for your patience.

@retrofox
Copy link
Copy Markdown
Author

Hi @zertosh Honestly I don't get how to implement this control using glob in a similar way that browserify does. I've been working in bin/args.js file: https://gist.github.com/retrofox/fe232d6ec6be4c970b3b

I don't know if you meant something like that so I did these changes to try to tune to your idea.
This implementation doesn't avoid my main purpose which is do not create instances of chokidar.watcher unnecessarily. Also it makes the process slower.

By another hand please let me insist on muy idea:

  • Use glob adds complexity unnecessarily that anymatch (just only one line)
  • anymatch supports globpattern too
  • watchify is using chikidar right now where it uses anymatch
  • using anymatch results are really good at least in my tests

@retrofox
Copy link
Copy Markdown
Author

What do you think to use minimatch ? https://github.com/isaacs/minimatch
node-glob use this module which is part of the npm core

@mattdesl
Copy link
Copy Markdown
Contributor

Is there a repo where we can reproduce this 15 second watchify times? It could represent a bug with chokidar/watchify.

@retrofox
Copy link
Copy Markdown
Author

@mattdesl I don't worry about these tests because ultimately it's a particular case. Maybe I'm doing something wrong. I've started to ignore other folders and it seems work ok. But the time process really gets better when it avoids create watcher instances.

@zertosh
Copy link
Copy Markdown
Member

zertosh commented Apr 14, 2015

@retrofox I don't mean to give you a hard time, but I'd really like to rule out the possibility of a watchify/chokidar bug - I don't just want to mask it, I'd like to find the root cause and fix that.

I think there's a misunderstanding on how watchify determines what files to watch. Watchify only watches files that your bundle explicitly requires. It doesn't matter that your project lives in a directory structure with a million files - if your dependency graph only has 200 modules, then only 200 watchers will be set up. And this setup only happens at the start. Whenever a file changes only that watcher is closed (and maybe reopened).

I've reiterated that creating chokidar instances is really cheap - it's a negligible cost. If you're having long build times - and that's your motivation for this change - then lets find the root cause of your problem instead of covering it up.

I did some profiling myself with the diff below, and the cost of the chokidar instances is negligible.

diff --git a/index.js b/index.js
index 88249528..0c14706a 100644
--- a/index.js
+++ b/index.js
@@ -2,6 +2,7 @@ var through = require('through2');
 var path = require('path');
 var chokidar = require('chokidar');
 var xtend = require('xtend');
+var Minimatch = require('minimatch').Minimatch;

 module.exports = watchify;
 module.exports.args = {
@@ -15,12 +16,16 @@ function watchify (b, opts) {
     var delay = typeof opts.delay === 'number' ? opts.delay : 600;
     var changingDeps = {};
     var pending = false;
+    var ignored;

     var wopts = {persistent: true};
     if (opts.ignoreWatch) {
-        wopts.ignored = opts.ignoreWatch !== true
-            ? opts.ignoreWatch
-            : '**/node_modules/**';
+        ignored = opts.ignoreWatch !== true
+                ? opts.ignoreWatch
+                : '**/node_modules/**';
+        ignored = [].concat(ignored).map(function(i) {
+            return new Minimatch(i);
+        });
     }
     if (opts.poll || typeof opts.poll === 'number') {
         wopts.usePolling = true;
@@ -93,6 +98,7 @@ function watchify (b, opts) {
     });

     function watchFile (file) {
+        if (ignored && ignore(file)) return;
         if (!fwatchers[file]) fwatchers[file] = [];
         if (!fwatcherFiles[file]) fwatcherFiles[file] = [];
         if (fwatcherFiles[file].indexOf(file) >= 0) return;
@@ -108,6 +114,7 @@ function watchify (b, opts) {
     }

     function watchDepFile(mfile, file) {
+        if (ignored && ignore(file)) return;
         if (!fwatchers[mfile]) fwatchers[mfile] = [];
         if (!fwatcherFiles[mfile]) fwatcherFiles[mfile] = [];
         if (fwatcherFiles[mfile].indexOf(file) >= 0) return;
@@ -122,6 +129,12 @@ function watchify (b, opts) {
         fwatcherFiles[mfile].push(file);
     }

+    function ignore(file) {
+        return ignored.some(function(mm) {
+            return mm.match(file);
+        });
+    }
+
     function invalidate (id) {
         if (cache) delete cache[id];
         if (fwatchers[id]) {

@zertosh zertosh mentioned this pull request Apr 19, 2015
4 tasks
@codeviking
Copy link
Copy Markdown

Turns out I duplicated this in: #232

Our concern wasn't performance, but an issue where it was watching files in node_modules and throwing a segfault when someone ran an npm update (which removed files it was watching).

I can't see why it would attach watchers to files which the user explicitly requests not to be watched..

@zertosh
Copy link
Copy Markdown
Member

zertosh commented Jun 13, 2015

@codeviking that's a very legitimate use case. thanks!

@zertosh
Copy link
Copy Markdown
Member

zertosh commented Jun 23, 2015

superseded by #237 and 7cfb8fa

@zertosh zertosh closed this Jun 23, 2015
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.

4 participants