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

fs/path: expose internal-api minimatch/glob under path module. #40731

Closed
kaizhu256 opened this issue Nov 5, 2021 · 14 comments · Fixed by #51912
Closed

fs/path: expose internal-api minimatch/glob under path module. #40731

kaizhu256 opened this issue Nov 5, 2021 · 14 comments · Fixed by #51912
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@kaizhu256
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Please describe the problem you are trying to solve.

i have a zero-dependency coverage-reporting-tool and would like to add a cli-option to exclude code-coverage based on file-glob-patterns, e.g.:

node jslint.mjs v8_coverage_report \
    --exclude=deps/*,test/* \
    npm run test

Describe the solution you'd like
Please describe the desired behavior.

expose ./deps/npm/node_modules/minimatch/minimatch.js under the builtin path module, e.g.:

import {minimatch} from "path";
minimatch("bar.foo", "*.foo") // true!
minimatch("bar.foo", "*.bar") // false!

Describe alternatives you've considered
Please describe alternative solutions or features you have considered.

i could re-implement ./deps/npm/node_modules/minimatch/minimatch.js into my zero-dependency-project, but i suspect there are many other projects out there that would benefit from exposing this api.

@targos targos added the feature request Issues that request new features to be added to Node.js. label Nov 5, 2021
@targos targos moved this to Pending triage in Node.js feature requests Feb 13, 2022
@isaacs
Copy link
Contributor

isaacs commented Feb 28, 2022

Update to this: I'm in the process of some updates to minimatch and glob to:

  1. Bring it into the modern era (classes, promises, iterating/streaming results, etc.)
  2. Improve performance and avoid ReDOS threats by just parsing and evaluating without building up massive regexps.
  3. Support some edge cases that have always been a problem (especially: nested extglob patterns).

Once that's done, I'll be advocating for including it in core.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Aug 28, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@meyfa
Copy link
Contributor

meyfa commented Oct 9, 2022

I think this would be a great addition, still! Can this issue be re-opened?

@MoLow
Copy link
Member

MoLow commented Oct 29, 2022

@isaacs I intend to work on this.
since your comment was posted 8 months ago can I assume you are not working on it as well?

@MoLow MoLow reopened this Feb 19, 2023
@MoLow
Copy link
Member

MoLow commented Feb 19, 2023

I am re-opening this issue since:

  1. it was raised again Request: mark test runner stable in Node 20.0.0 #46642 (comment)
  2. mini-match seems to have had a major release lately

@isaacs is this still true?

Once that's done, I'll be advocating for including it in core.

@kaizhu256
Copy link
Contributor Author

  • just fyi, the original test-coverage-project that raised this issue been fixed (still would be a useful nodejs feature though)
  • here's standalone, 300-line-gistfile of minimatch-alternative i've been using to batch-glob large number of files for test-coverage

@github-actions github-actions bot removed the stale label Feb 20, 2023
@isaacs
Copy link
Contributor

isaacs commented Apr 7, 2023

Yes, glob and minimatch have both been significantly updated lately. They're much faster, better designed, more faithfully implement bash pattern expansion semantics, and ship types.

I'm still not sure it makes sense to pull into node core though. Making glob fast did require a lot of moving parts. But the license allows it, so have at it if y'all want it in. I see no reason to recommend otherwise, and would be happy to help out if someone attempts this and runs into issues.

It might be desirable to make the glob stream interfaces node streams (or web streams) if so, as they're minipass streams to support sync globbing. It'll cost some perf to do that, tho. (Or we could talk about pulling in minipass, which wouldn't be a bad idea either, but that's of course a much bigger change.) Minimatch also exports its parser on minimatch.AST, which can be helpful if you want to use some parts but assemble it differently.

If you only need a subset of the bash globbing behavior, and especially if you don't need hot path performance or windows support, then a couple hundred line js module can definitely suffice. But I would not recommend pulling such a glob subset into core, unless the node team is interested in a never ending stream of bug reports for every edge case where it'll diverge from shell behavior. Having an incorrect or incomplete glob module in core is arguably worse than not having one at all.

Another completely different route to globbing in node core would be to explore doing it in C, by using the same actual code that expands patterns in bash or zsh. I'm not sure if their licenses would be compatible, of course, and bash globs are pretty slow on large folder trees, compared to node-glob or especially fastglob. But you can't get more consistent than "using the same code".

But anyway, yeah, if it was up to me, I'd say just let glob live in userland. It's fine there.

@isaacs
Copy link
Contributor

isaacs commented Apr 7, 2023

@kaizhu256 If you really want your tool to support globs faithfully, and remain "zero-dep", there's an easy solution.

npm install glob@latest --no-save
mv node_modules vendor
echo 'module.exports = require("../vendor/glob")' > lib/glob.js

😜

This was referenced Apr 8, 2023
@benjamingr
Copy link
Member

But anyway, yeah, if it was up to me, I'd say just let glob live in userland. It's fine there.

The issue mostly is that it's increasingly a common request needed by other Node.js features (like the test runner) that are seeing bigger and bigger adoption.

@isaacs
Copy link
Contributor

isaacs commented Apr 10, 2023

@benjamingr Then the thing to do, imo, is to bundle node-glob (if you want as correct/complete results as possible) or fast-glob (if you want as fast performance as possible), and call it a day. The difference is minor, node-glob is still very fast, and fast-glob is still very correct. Both are a much better fit for node than libglob can ever be.

@benjamingr
Copy link
Member

Personally I agree and it's also likely better in terms of ease of understanding the code for most contributors, security and updates.

If Node wants to expose it as a built in API no one is stopping us from upstreaming changes making things like "throw Node's own errors" or "use primordials if node-glob is built with this or that flag" to node-glob or whatever we choose (in the error case Node can just wrap the errors like it does with other deps)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@boneskull
Copy link
Contributor

boneskull commented Mar 20, 2024

Please allow me to necro this issue because of #51912

My current workaround:

glob -c "tsx --test" "./src/test/*.spec.ts"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@isaacs @kaizhu256 @boneskull @benjamingr @targos @meyfa @MoLow and others