Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Deprecate fileset #638

Closed
mklabs opened this issue Jun 2, 2016 · 8 comments
Closed

Deprecate fileset #638

mklabs opened this issue Jun 2, 2016 · 8 comments

Comments

@mklabs
Copy link

mklabs commented Jun 2, 2016

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
Copy link
Owner

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
Copy link
Author

mklabs commented Jun 5, 2016

@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
Copy link
Author

mklabs commented Jun 5, 2016

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.

johanneswuerbach added a commit to johanneswuerbach/testem that referenced this issue Jun 18, 2016
fileset is deprecated gotwarlost/istanbul#638
and glob 7 supports everything we need
graingert added a commit to graingert/istanbul that referenced this issue Jun 21, 2016
graingert added a commit to graingert/istanbul that referenced this issue Jun 21, 2016
@gilligan
Copy link

gilligan commented Jul 8, 2016

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
Copy link
Author

mklabs commented Jul 9, 2016

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

Let this issue be closed for the moment.

@mklabs mklabs closed this as completed Jul 9, 2016
@lo1tuma
Copy link

lo1tuma commented Jul 10, 2016

@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
Copy link
Author

mklabs commented Jul 10, 2016

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.

@mklabs mklabs reopened this Jul 10, 2016
popomore pushed a commit to popomore/istanbul that referenced this issue Jul 28, 2016
popomore pushed a commit to popomore/istanbul that referenced this issue Jul 28, 2016
@maxkoryukov
Copy link

Looks, like there is a solution: #673

gotwarlost added a commit that referenced this issue Aug 21, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants