deps: add minimatch as a dependency

[MoLow]

Refs: #47490
Refs: #47486

TLDR of the discussions in those two PRs is:

• it does not make much sense to re-implement glob matching in core, minimatch is a battlefield-tested solution for this.
• exposing minimatch as-is via fs.glob or path.glob has many issues (doesn't use primordials, doesn't use node's built-in errors, its API might change, it is not bound to node's semver)

this PR adds minimatch as a dependency with the desire to currently only use internally without exposing.
if we ever decide to expose it - we can follow up on this and address the issues raised.

[nodejs-github-bot]

Review requested:

• @nodejs/actions
• @nodejs/tsc

[MoLow]

CC @isaacs

[marco-ippolito]

LGTM for the dependency update side

[tniessen]

As @anonrig mentioned in #45127 (review), we probably should not add dependencies without having any code that uses them.

with the desire to currently only use internally without exposing

How is this going to solve the issues, e.g., lack of primordials?

[MoLow]

we probably should not add dependencies without having any code that uses them.

will be added in follow-up PRs. mostly needed for test runner and watch mode

How is this going to solve the issues, e.g., lack of primordials?

It won't, but both test runner and watch mode code runs in an isolated demon process that won't be affected by user-land code.
that being said, I agree with @benjamingr's suggestion to upstream adjustments making this more suitable for other use-cases

[cjihrig]

test runner and watch mode code runs in an isolated demon process that won't be affected by user-land code.

Is that true even with preloads (--require)?

[anonrig]

test runner and watch mode code runs in an isolated demon process that won't be affected by user-land code.

What keeps a new pull request for using this vendored package in anywhere in Node, where user-land code might get affected? Can we have some sort of protection, like an eslint rule?

This is going to be the first package that we vendor as a JS package that isn't maintained by the Node.js organization. My main concerns are lack of primordials and the maintainability of this package.

Question: Why didn't we vendor test runner, watcher etc in the past?

[jasnell]

This is going to be the first package that we vendor as a JS package that isn't maintained by the Node.js organization.

Not quite. We've had acorns in there for a while, and of course there's the old punycode module.

[benjamingr]

(I did not talk to Moshe before posting this so if you find this annoying - sorry Moshe!)

I think the burden we're putting on contributors and Moshe in particular with the whole glob feature is unreasonable. He has tried 3 approaches now from scratch after months of discussions and a lot of drive-by feedback and limited availability.

To be clear everyone here is interacting in good faith and trying to do what's right for the project - but this sort of push-back for an experimental feature can be super draining and frustrating (well, now not even a feature - just adding the code so it can be used elsewhere).

I think he is being extremely patient here volunteering his time for this feature because it's a common feature request by users. I think he has also shown (many times before) he is willing to backtrack on experimental features and additions - just so we have a concrete example: assert.snapshot which was added as experimental and then reverted.

I think we can add glob path support for watch/test experimentally, let people play with it, see how big issues like primordials/error codes are in practice (primordials for example would be less of an issue for this since it's for dev-time features and not for a web standard but this can absolutely be iterated on).

If feedback arises that shows this approach isn't good and for example the libglob approach is - we can switch, remove this feature or do something else but I would personally strongly prefer we move forward with something and move back or in a different direction if we need to. Ideally with user feedback.

(Again to iterate, all the feedback on these PRs is good, valuable and in good faith - the critique is more about our process)

[anonrig]

When I think over, I agree with @benjamingr's reply and retract my concerns (if they are blocking). I appreciate @MoLow's work, and the patience they've put into this issue.

[isaacs]

I agree with @benjamingr's take, and also applaud @MoLow's patience through this. I hope that the reservations I expressed weren't too demoralizing. I completely agree the important thing is to land something in whatever way can deliver some value and not commit the project to anything regrettable.

Since it seems it's happening, let's make it happen. 😂 path.match and fs.glob belong in core. That's how I ended up stuck maintaining a glob library in the first place, back in 2009 when ry said he thought it would be a good addition.

I've started porting primordials into userland in a more complete way so that I can see how big a lift it'd be to port minimatch, glob, et al over to using it.

Then pulling in patches upstream should be a matter of just removing the import { primordials } from 'node-primordials' line (assuming that node's internal primordials API surface doesn't change too rapidly, but it looks like it's fairly stable).

I'm somewhat concerned about the perf impact, but it would be very easy to have a "fast" mode that uses the same method names but doesn't actually harden everything under layers of bind.bind(call) etc: https://github.com/isaacs/node-primordials Benchmarks will make the decision clear.

There are some other one-off examples I saw that are similar, but I figured it'd be nice to just have a somewhat complete one of these that we could use in similar cases.

Error codes are much less of a lift. I try to mostly follow node's patterns on that anyway, but if there's some documentation of how they're supposed to look, or if someone wants to send patches to minimatch/glob/etc to node-ify the errors, I'm 100% open to it.

[cjihrig]

Opened #47521 to discuss making Node's error system available to userland in case that's something of interest here.

[MoLow]

thanks, @benjamingr ❤️ I don't feel like any of the feedback / pushback was posted in bad faith

As per some of the concerns raised above:

Is that true even with preloads (--require)?

in watch mode yes, but not in test runner, but we can probably disable require on the test runner wrapping process (the second argument of prepareMainThreadExecution will disable it):

node/lib/internal/main/watch_mode.js

Line 31 in 6dcbf8b

prepareMainThreadExecution(false, false);

node/lib/internal/main/test_runner.js

Line 12 in 6dcbf8b

prepareMainThreadExecution(false);

Since it seems it's happening, let's make it happen. 😂
I've started porting primordials into userland in a more complete way so that I can see how big a lift it'd be to port minimatch, glob, et al over to using it.

Amazing. I really appreciate this effort!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51205/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51209/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/51215/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/47499
✔  Done loading data for nodejs/node/pull/47499
----------------------------------- PR info ------------------------------------
Title      deps: add minimatch as a dependency (#47499)
Author     Moshe Atlow  (@MoLow)
Branch     MoLow:add-minimatch-as-dep -> nodejs:main
Labels     meta, needs-ci, dependencies
Commits    2
 - deps: add minimatch as a dependency
 - CR
Committers 1
 - Moshe Atlow 
PR-URL: https://github.com/nodejs/node/pull/47499
Refs: https://github.com/nodejs/node/pull/47490
Refs: https://github.com/nodejs/node/pull/47486
Reviewed-By: Marco Ippolito 
Reviewed-By: Robert Nagy 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47499
Refs: https://github.com/nodejs/node/pull/47490
Refs: https://github.com/nodejs/node/pull/47486
Reviewed-By: Marco Ippolito 
Reviewed-By: Robert Nagy 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - CR
   ℹ  This PR was created on Mon, 10 Apr 2023 22:27:24 GMT
   ✔  Approvals: 2
   ✔  - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/47499#pullrequestreview-1378787712
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/47499#pullrequestreview-1378830404
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-04-13T19:15:00Z: https://ci.nodejs.org/job/node-test-pull-request/51215/
- Querying data for job/node-test-pull-request/51215/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4759944252

[MoLow]

Landed in f536bb0