glob-based include/exclude functionality

[bcoe]

A few slight tweaks to Lalem001's awesome work on glob-based excludes:

• I've switched to (at this time an older) version of micromatch. This works with leading ./ so that using npm link does not break exclusions.
• I've added support for include along with exclude.

Reviewers: @Lalem001, @davidchase, @novemberborn

[Lalem001]

I see a potential problem with the include implementation.

If both include and exclude are defined, shouldInstrumentFile will ignore the exclude completely.

Also, if someone wished to include the entire contents of their source directory without using --all, this implementation of include will not do that. It will only specifically include whatever was required.

[jamestalmage]

Personally, I have come to prefer multimatch, you write specific includes, and can exclude with negated (!) patterns at any time.

If you are going to provide both include and exclude, my opinion is that exclude should take priority (i.e. if it matches exclude then it is excluded no matter what include says) include-exclude works that way.

[novemberborn]

There's a note paragraph at the end of the readme which is in italics, yet this is a quote. Would be nice to be consistent.

[novemberborn]

Also, if someone wished to include the entire contents of their source directory without using --all, this implementation of include will not do that. It will only specifically include whatever was required.

I guess the README could make that more clear (though the section after config.include covers it).

[novemberborn]

If you are going to provide both include and exclude, my opinion is that exclude should take priority (i.e. if it matches exclude then it is excluded no matter what include says)

There are default exclusions though. Would be good to allow a particular module to be covered, even if generally all modules are excluded.

[Lalem001]

There are default excludes though. Would be good to allow a particular module to be covered, even if generally all modules are excluded.

@jamestalmage hit on this a bit. It would be possible to have exclusion negations with a leading!. ESLint does this. For example, since node_modules is always excluded, a negation to allow glob to be covered could be something like:

{
  exclude: ['!node_modules/glob']
}

This is effectively the same as the current idea of include, where a file is included despite having been excluded in other patterns.

[jamestalmage]

Yep, I think that's why it may make sense to have separate include/exclude globs, with the following defaults:

{
  "include": "**/*",
  "exclude": ["**/node_modules/", "**/test/**", "**/test.js"]
}

[Lalem001]

@jamestalmage That example is effectively what the --all flag will accomplish with this PR.

What would be nice, and I was hoping to accomplish in a future PR, is to have something like:

{
  include: ['src/**/*.js', 'bin/**/*.js']
}

This is not precisely possible with the current include implementation.

[bcoe]

the two use-cases that I'd like to solve are:

1. a project has many files, but only cares about covering one -- for instance a codebase that transpiles quite a few files into a single asset (moment does this) -- I see include solving this problem.
2. a project that has many files, and only wants to exclude a few -- this is the more common case, and I think is solved by exclude.

I wonder what the most elegant approach to solve this is.

[jamestalmage]

Right, I think what I suggested before:

var defaults = {
  include: everything,
  exclude: node_modules_and_common_test_paths
};

Our algorithm for matching is:

var fileMatches = includes.matches(file) && !excludes.matches(file)

1. If they provide their own includes we replace the default array, this makes a whitelist approach easy (but still provides automatic exclusion of node_modules, just in case they carelessly do something like **/lib/*.js).
2. If they provide their own excludes we append to the default array. This makes excluding one/two other files in addition to the sensible defaults easy.
3. If they really want to include something in node_modules, they have to provide a negation for it (since we aren't allowing them to replace the default excludes). It's a bit weird, but it is also a super rare need. Even then, it's probably the simplest (fewest characters) for the user once they understand. We obviously would need to document this with an example.

@bcoe, I think this method gives you the best answer to your request:

1. a project has many files, but only cares about covering one -- for instance a codebase that transpiles quite a few files into a single asset (moment does this) -- I see include solving this problem.

Just add a glob for that file to include.

1. a project that has many files, and only wants to exclude a few -- this is the more common case, and I think is solved by exclude.

Just add that file to exclude, since we are appending it to the default exclusion list (instead of replacing the default), you won't suddenly be covering node_modules, etc when you provide an exclusion pattern.They can use negation patterns for exclude in the corner cases where they actually want to exclude node_modules

[bcoe]

@jamestalmage I am sold \o/

[bcoe]

@jamestalmage I think this is ready to land, I've tested thoroughly at this point.

[Lalem001]

Sorry for the last minute comment. Please bear with me, but I would like to air out my thoughts.

I see files being loaded in two distinct modes right now (prior to 3db354b):

• Files required after wrap has been invoked (default)
• Files picked up by glob when the --all flag is present

In the default mode, include and exclude are only working against a very limited set of files.
Include is not actually adding any files that were not required.

In the second mode, all files that are not explicitly excluded are included.
Include, I think, would have little effect here.

I was thinking that include can instead replace the **/*.js that is currently in addAllFiles. Basically have a new function addFiles and addAllFiles simply calls the prior with **/*.js. Then include can be sent to addFiles.

This way include can specifically add files that are not required.

Does that make sense? Or is that not the vision for include that you had in mind?

[bcoe]

@Lalem001 let's open a discussion about addAllFiles in a separate thread, I would like to land a first pass of these glob changes, and get a release out of nyc out the door -- we have several important patches ready to go live.

[Lalem001]

@bcoe understood. Go for it

[jamestalmage]