search

gotwarlost / istanbul

Yet another JS code coverage tool that computes statement, line, function and branch coverage with module loader hooks to transparently add coverage when running tests. Supports all JS coverage use cases including unit tests, server side functional tests and browser tests. Built for scale.
Other
8.7k stars 786 forks source link

Deprecate fileset #638

Closed mklabs closed 8 years ago

mklabs commented 8 years ago

Hi!

I'm opening this issue to start a thread on the possibility to deprecate https://github.com/mklabs/node-fileset

I recently merged a pending PR from @isaacs (mklabs/node-fileset#22), which

may or may not be a breaking change from fileset's pov. (changelog)

to quote Isaacs. So I bumped filset to a major 1.0 version which should save you from any trouble.

I really really appreciate that you choose to rely on it and build istanbul on top of it. But, to be honest, fileset is a package I wrote a long time ago and never really used it. I always either used just glob with the ignore pattern or redone the filtering on top of globs results before glob 5.

I'm considering deprecating the package in favour of newer, better similar packages like @sindresorhus https://github.com/sindresorhus/globby, or simply glob itself with the options.ignore patterns.

This should help with performance (as the minimatch ignore is done internally by glob, avoiding one costly glob call on all ignore patterns) and issues like #556

I can maybe help in the migration process by opening a PR to switch from fileset to globby, or just glob's with options.ignore.

Let me know :)

Thanks for building Istanbul.

gotwarlost commented 8 years ago

Sorry, I haven't been paying attention to istanbul for some time now. I hope to pick it up in mid-June.

I've always wanted to the remove the fileset dependency (for one thing, the current code "matches" by loading all file names into a map rather than doing it lazily).

I would love to get a PR that changes this behavior and loses the fileset dependency.

mklabs commented 8 years ago

@gotwarlost I have 10 or so failing tests, is it ok if I just focus on the lib/util/file-matcher.js file and related tests ? (as long as those failed test remains to 10)

I was really please to see only one occurence of fileset :)

mklabs commented 8 years ago

I feel like changing fileset feature-set a bit, and add fs helpers I often need, generally as a mixin. You might want to checkout fileset v2 when I'll have the chance to work on it.

There's less value in a glob / minimatch combo now that Glob supports it out of the box, i'd rather move fileset into a fs utils library.

gilligan commented 8 years ago

On top of that istanbul depends on fileset 0.2.x which in turn depends on minimatch 0.x:

npm WARN deprecated minimatch@0.3.0: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue

Another reason why it would be nice to replace it or (if more feasible on the short term) update to a more recent version. Actually I tried switching to the latest version and there was only one failing test: should cover everything under the sun when default excludes are suppressed.

mklabs commented 8 years ago

As I can see fileset have been replaced with plain Glob, which is good :) 👍

Let this issue be closed for the moment.

lo1tuma commented 8 years ago

@mklabs Actually fileset is still a dependency of istanbul. There is an open PR #648 which would replace fileset with glob but it hasn’t been merged yet. So this issue still remains.

mklabs commented 8 years ago

Ok reopening :)

I can't promise anything, but I'll try to issue a small PR this week to update fileset to 2.0.x.

maxkoryukov commented 8 years ago

Looks, like there is a solution: #673