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

Add --include cli option #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

levithomason
Copy link
Collaborator

Updated version of #91. Also added a fix to the engines in package.json.

@levithomason
Copy link
Collaborator Author

levithomason commented Apr 21, 2016

I think this needs more review and possible updates. My intuition is that --include should work in combination with the directories being watched. Given:

/foo
  bar.js
  bar-test.js

I'd expect to be able to watch foo and add a pattern to target only my tests:

node cli.js 'echo "test changed"' foo --include *-test.js

This doesn't work because the include logic is comparing the abs file path to the pattern:

minimatch(fileName, argv.include)

Haven't thought this through, but it seems we should be testing if any watched absolute path includes the pattern. Or, allow any parent directories via wildcard:

In either case if it is not updated you have to write:

node cli.js 'echo "test changed"' foo --include **/foo/*-test.js

@pgraham
Copy link

pgraham commented Apr 25, 2016

I agree and think that this is unintuitive behaviour. In my case the pattern I'm using is --include **/*.less. No specific directory but the same general idea.

After thinking about this for a couple of days I think that the simplest solution is to inspect the include pattern for a path separator. If one is found, apply the pattern to the relative path from the current working directory (strip cwd from absolute path). If no path separator is found, apply the pattern to the filename only. This would lead to commands like:

node cli.js 'echo "test changed"' foo --include *-test.js

or if you wanted to restrict the pattern to a subdirectory:

node cli.js 'echo "test changed"' foo --include spec/**/-test.js

The only complication that jumps out at me is if the pattern has an escaped path separator. Thoughts?

@levithomason
Copy link
Collaborator Author

Cool, I'll do some experimenting with this and let you know.

Semi-related, I've also just run across micromatch which seems to be "a highly drop in replacement for minimatch". I might check this out as well.

Thanks for the brain cycles on this.

@levithomason
Copy link
Collaborator Author

levithomason commented Apr 26, 2016

OK, here's what I found. If no directories are provided then the path file paths passed to the filter are absolute. If any directories are passed, then the file paths passed to the filter are relative. This alone seems like an issue, but will save that for another PR.

With directories
› node cli.js 'echo "test changed"' foo --include *-test.js

What I'm thinking is that if directories supplied, they should be traversed, looking for a pattern match. If any filename relative to any directory argument matches the pattern, then it is a match.

I've added some example code and logging for trying this out:

> Watching foo
{ dir: 'foo',
  absDir: '/Users/levithomason/src/watch/foo/',
  fileName: 'foo/bar-test.js',
  relFileName: 'bar-test.js' }

isMatch  pattern   fileName
true    *-test.js  foo/bar-test.js
--------------------------------------------------------------------------------
{ dir: 'foo',
  absDir: '/Users/levithomason/src/watch/foo/',
  fileName: 'foo/bar.js',
  relFileName: 'bar.js' }

isMatch  pattern   fileName
false    *-test.js  foo/bar.js
--------------------------------------------------------------------------------

Without directories
› node cli.js 'echo "test changed"' --include *-test.js

In this case, we'd use what ever pattern you passed us against the absolute file paths. So the above would no longer match, because it only looks for *-test.js files, not **/*-test.js files.

{ dir: '/Users/levithomason/src/watch',
  absDir: '/Users/levithomason/src/watch/',
  fileName: '/Users/levithomason/src/watch/foo/bar-test.js',
  relFileName: 'foo/bar-test.js' }

isMatch  pattern   fileName
false    *-test.js  /Users/levithomason/src/watch/foo/bar-test.js
--------------------------------------------------------------------------------
{ dir: '/Users/levithomason/src/watch',
  absDir: '/Users/levithomason/src/watch/',
  fileName: '/Users/levithomason/src/watch/foo/bar.js',
  relFileName: 'foo/bar.js' }

isMatch  pattern   fileName
false    *-test.js  /Users/levithomason/src/watch/foo/bar.js
--------------------------------------------------------------------------------

This doesn't account for sub directories right now and I'm sure other things. But, the approach I'm thinking should be "if dirs, scan them with the pattern, else use the pattern on abs paths". It's late so I'll let this one sink in a bit before doing anything.

Any feedback greatly appreciated.

@akanieski
Copy link

Any chance we can get this merged in? :-)

@levithomason
Copy link
Collaborator Author

@akanieski right now I don't think this PR handles sub directory globs properly. I have no time to come back to it at present. It is on my list of things to do.

I've really been thinking of overhauling the watch system entirely with more robust and reliable packages.

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