Skip to content

Do not ignore me#193

Closed
retrofox wants to merge 20 commits intobrowserify:masterfrom
retrofox:master
Closed

Do not ignore me#193
retrofox wants to merge 20 commits intobrowserify:masterfrom
retrofox:master

Conversation

@retrofox
Copy link
Copy Markdown

@retrofox retrofox commented Apr 8, 2015

The title is just a joke. :-D because I've working in how to improve ignored files processing.

regexp

Allow give a regexp in ignore-watch parameter. As you know chokidar allow pass a regexp in its ignored option parameter:

// One-liner for current directory, ignores .dotfiles
chokidar.watch('.', {ignored: /[\/\\]\./}).on('all', function(event, path) {
  console.log(event, path);
});

However we can't do it through of ignore-watch.
So in this PR it could be a string:

watchify index.js . --ignore-watch="**/node_modules/**" -o build.js

or a regexp

# Ignore css files
watchify index.js . --ignore-watch="*/\.css/" -o build.js

Keep in mind that ignore-watch always is a string but now if it is wrapped in / chars then a RegExp will be created. Look at this complex ignoring condition:

# my Makefile
# Just watch .jade, .json and .js files into lib/ folder
IGNORE_WATCH := "/^((?!my-project\/lib\/.*\.j(ade|son|s)).)*$$/"
watchify . --ginore-watch=$(IGNORE_WATCH) -o build.js

Multiple ignore-watch parameters

This PR allows give more that one --ignore-watch parameters.

# my Makefile
# Just watch .jade, .json and .js files and ignore `node_modules` folder
watchify
  .
  --ignore-watch="/^((?!\.j(ade|son|s)).)*$$/" \
  --ignore-watch="**/node_modules/**" \
  --oufile build.js

Improve ignoring

I've added conditionals in some places of index.js to ignore completely those files that not match with ignore-watch param (when it is a regexp).
It improve considerably the compiling time and avoids to call to compiling process every time a file is edited no matter what type of file being edited (as now it does).

Test

I've created bin_ignore_watch_regexp.js testing file.

@retrofox
Copy link
Copy Markdown
Author

retrofox commented Apr 8, 2015

Screenshots of my current work. I'm using saving a jade file and it's compiled through of jadeify.

now

With these changes

@zertosh
Copy link
Copy Markdown
Member

zertosh commented Apr 9, 2015

Thanks @retrofox, I really really appreciate it when users of a project are involved and contribute back - especially when they have tests heh. So please don't get discouraged from further contributions. Here are some thoughts:

  1. I don't think watchify should be in the business of handling ignores. If it were, then I'd remove passing anything to chokidar. Having two places where the ignore logic happens is just asking for trouble.
  2. Not sure if you know this, but chokidar's anymatch also takes an array. So you can do watchify index.js --ignore-watch='**/static-libs/*.js' --ignore-watch='**/node_modules/react/**' -o built.js. Multiple option switches with the same name get turned into an array. I'm not sure how to express a negative as glob, it's something with !, but it's doable too.
  3. If you're having those crazy long rebuilds and you're using the same commands as in your example, then that's your problem right there. Remove the period. Instead of watchify index.js . --ignore-watch="*/\.css/" -o build.js, do watchify index.js --ignore-watch="**/*.css" -o build.js.

@retrofox
Copy link
Copy Markdown
Author

retrofox commented Apr 9, 2015

Hi @zertosh for your thoughts. Let me add some comments:

I don't think watchify should be in the business of handling ignores. If it were, then I'd remove passing anything to chokidar. Having two places where the ignore logic happens is just asking for trouble.

I know isn't best solution but watchify should controller when make (or not) a new watcher instance.
Here https://github.com/substack/watchify/blob/master/index.js#L96-L98 these are the only conditions when a new watcher (chokidar watcher) is created. So a new one is created regardless if the file should be ignored or not.
Also we are doing some tasks when a file changes in the subscribed functions to change event. We could check if this file is ignored to avoid those tasks.
And finally, in some way, watchify is adding ignoring stuff in its code. https://github.com/substack/watchify/blob/master/index.js#L20-L23

Not sure if you know this, but chokidar's anymatch also takes an array. So you can do watchify index.js --ignore-watch='/static-libs/*.js' --ignore-watch='/node_modules/react/**' -o built.js. Multiple option switches with the same name get turned into an array. I'm not sure how to express a negative as glob, it's something with !, but it's doable too.

Yes, but chokidar also accepts a regex in ignored parameter. Right now we can't do it through of --ignore-watche parameter. Anyway I could solve my issue using this notation. I just have to learn how to do it. :-D But I think is important don't limit ignored parameter when it is passed to chokidar.

If you're having those crazy long rebuilds and you're using the same commands as in your example, then that's your problem right there. Remove the period. Instead of watchify index.js . --ignore-watch="/.css/" -o build.js, do watchify index.js --ignore-watch="__/.css" -o build.js.

I think the problem is because we aren't handling when a new watcher should't be created (yes, again).
Let's say we have an app folder with 10k files, and we wanna ignore 9k. It doesn't matter because watchify will create 10k watcher instances anyway.
https://github.com/substack/watchify/blob/master/index.js#L153
Create unnecessarily 9k of watcher instances no make sense.

@retrofox retrofox changed the title Do not ignore me [wip] Do not ignore me Apr 9, 2015
@retrofox
Copy link
Copy Markdown
Author

retrofox commented Apr 9, 2015

Doing some changes. I've added [wip] to title

@retrofox
Copy link
Copy Markdown
Author

retrofox commented Apr 9, 2015

Ok. I've added anymatch module. it's used in chokidar too. Whit this one I've simplified the login to check when watchify should create a new watcher or when make some operations when a file changes.
Also it's very useful to allo multiple --ignore-watch parameters:

# my Makefile
# Just watch .jade, .json and .js files and ignore node_modules folder
watchify
  .
  --ignore-watch="/^((?!\.j(ade|son|s)).)*$$/" \
  --ignore-watch="**/node_modules/**" \
  --oufile build.js

@retrofox retrofox changed the title [wip] Do not ignore me Do not ignore me Apr 9, 2015
@zertosh
Copy link
Copy Markdown
Member

zertosh commented Apr 9, 2015

these are the only conditions when a new watcher (chokidar watcher) is created. So a new one is created regardless if the file should be ignored or not.

Yes. But chokidar instances are super cheap. At most, you're talking about bytes of memory per instance. Even if you have 9k instances, that's probably less than 1MB. And those are just "dead" instances, they don't do any work. The expensive part is the file system watching. When a file is ignored, chokidar won't actually setup any fs watchers. See #65 for more context.

Also we are doing some tasks when a file changes in the subscribed functions to change event.

Not really because no fs watchers were setup by chokidar, so the update function never gets called.

but chokidar also accepts a regex in ignored parameter.

The way we use chokidar makes this an implementation detail. The standard for filename patterns is globs. anymatch also takes functions, but aren't going to handle that. If someone really wants to pass in a regex or function, they can use the API instead of the CLI.

And finally, in some way, watchify is adding ignoring stuff in its code.

It's setting a sane default value that chokidar will then decide on. What you proposed was preempting chokidar's matching logic by doing it in watchify in some cases - double logic.

Anyway I could solve my issue using this notation. I just have to learn how to do it.

globs are very important notation. They're universally supported across languages, shells, and even OSes.

But I think is important don't limit ignored parameter when it is passed to chokidar.

It's only a CLI limitation (via the API you can do whatever you want) for 2 reasons: globs are the standard for shell commands, and inventing your own pattern for sniffing out regexs from strings is a Bad Idea.

Create unnecessarily 9k of watcher instances no make sense.

As mentioned above, this isn't a big deal because the fs watchers aren't created if chokidar matches the ignored pattern.

Ok. I've added anymatch module. it's used in chokidar too.

Sorry dude. I recommend you look at node-glob.


There are also a bunch a changes that are just noise in your commits - like whitespace and comma changes. Those changes, although an improvement, make the git history hard to read.

Unless there's a good reason why globs with chokidar don't work, I'd only then consider switching to handling ignoring inside of watchify, and only if it's implemented in the same way that browserify already handles file patterns.

@retrofox
Copy link
Copy Markdown
Author

ok @zertosh. Thanks for your feedback. I've cleaned a lot this PR. It's ignoring files using anymatch module. With this improvement we don't create unnecessarily chokidar watchers. I'll keep working in regexp in another PR and who know.

@retrofox
Copy link
Copy Markdown
Author

I'll close this one and start a new one. Apologies.

@retrofox retrofox closed this Apr 13, 2015
@zertosh zertosh mentioned this pull request Apr 14, 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.

2 participants