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

The dependency on micromatch package should be removed #7591

Closed
prahladyeri opened this issue Jun 26, 2018 · 7 comments
Closed

The dependency on micromatch package should be removed #7591

prahladyeri opened this issue Jun 26, 2018 · 7 comments

Comments

@prahladyeri
Copy link

prahladyeri commented Jun 26, 2018

I've observed that you have a dependency on micromatch npm package which pulls in additional trivial packages like nanomatch, is-odd, is-even and is-number. The only place you're using this module is in the /lib/optimize/SideEffectsFlagPlugin.js which should be trivial to implement in pure JS.

return mm.isMatch(moduleName, flagValue, {
				matchBase: true
});

I hope you'll do it and remove this dependency soon. I also hope you understand that adding dependencies for trivial functionality like this is a worst practice presently plaguing the javascript world and the idea of "don't repeat yourself" is being carried too far with packages like is-odd and is-even.

@montogeek
Copy link
Member

That isMatch function is doing more than you think.
It also have a internal cache which is an optimization.

@davidtimovski
Copy link

I agree about the isMatch function. I'm waiting to see the justification of is-odd, is-even, and is-number.

@sokra
Copy link
Member

sokra commented Jun 26, 2018

Send a PR

@iamarkadyt
Copy link
Contributor

iamarkadyt commented Jun 27, 2018

Hmm, should globbing then support only a limited set of wildcards?
Plugin unittest tests with *, **, !, [a-z], {a, ..., n} and [[:lower:]] only.
Otherwise dep use seems quite justified...?

@sokra
Copy link
Member

sokra commented Jun 27, 2018

any limitation would be a breaking change. So only try to reduce dependencies without reducing functionality. You could also consider sending a PR to micromatch, or dependencies, to remove is-odd etc.

@jonschlinkert
Copy link

jonschlinkert commented Jun 27, 2018

I'm the author of micromatch.

  • is-odd was removed from the entire micromatch tree months ago.
  • is-number checks string values that are parsed from bash-like ranges in brace and bracket patterns in globs. edit: This is not something that typeof val === 'number' will work for. Obviously, there are many ways to check for a number in JavaScript, but given that I'm the author of both libraries I trust them both and know what result should be expected. If you disagree with my choices in the actual code, create an issue and tell me how it should be done differently. It's silly to create issues about something like this as a broad sweeping generalization, like "It uses a dependency" without examining what the actual code does.

You could also consider sending a PR to micromatch, or dependencies, to remove is-odd etc.

This would be fantastic. We're actively working to improve micromatch. As long as all 36k unit tests continue passing and we keep parity with bash, we'll take any PR that improves the performance or reduces code. Reducing deps and leveraging new es syntax is a top priority.

@sokra
Copy link
Member

sokra commented Jun 27, 2018

Great, so I guess we can close this one.

@sokra sokra closed this as completed Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants