Review requested:

• @nodejs/gyp

CC @anonrig WDYT?

You have to be careful as both bash and Linux are under the GPL license

This can't be merged as-is for the reason @targos mentions. I suggest closing this and starting over from a clean slate.

@MoLow I think this is a better approach than the previous one. Thank you for valuing my opinion. Happy help on the C++ side.

@targos @bnoordhuis according to the source code this specific module is licenced under MIT as well:
https://github.com/torvalds/linux/blob/cdc9718d5e590d6905361800b938b93f2b66818e/lib/glob.c#L10

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @MoLow.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

I have played around with this a bit, and the globbing algorithm is much more limited than I have thought, I am looking into other alternatives

@Trott @joesepi (pinging because you're our OpenJS CPC delegates), can either of you help with getting in touch with legal from the Linux Foundation to make sure the license was understood correctly and we are in compliance?

I remember that once upon a time in the Node.js foundation days that's something that wasn't too hard to do and we'd all like to avoid potential issues with this.

@Trott @joesepi (pinging because you're our OpenJS CPC delegates), can either of you help with getting in touch with legal from the Linux Foundation to make sure the license was understood correctly and we are in compliance?

Yes, I'll try to loop in the appropriate Foundation folks.

@Trott @joesepi (pinging because you're our OpenJS CPC delegates), can either of you help with getting in touch with legal from the Linux Foundation to make sure the license was understood correctly and we are in compliance?

@rginn

this implementation lacks some features that minimatch has such as brace expansion

Given that the original feature request (#40731) as well as the previous PR (#47486) specifically targeted minimatch/glob, could you be more specific as to the differences, especially with respect to different operating systems? This part of @isaacs' #40731 (comment) sounds like a proper implementation would be rather complicated:

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.

Do not assume that any particular glob implementation in C will be faster. The pattern expansion implementation found in bash and zsh is likely to have serious performance issues in node. Shell expansion is optimized for intuitive shell UX and correct behavior in low-memory systems, and will likely be a footgun if anyone puts it to serious use in a node program at runtime. Node programs need to go fast, and they're memory hogs, it's the exact opposite of what Bash's glob is designed for.

I have benchmarked node-glob, fast-glob, and globby extensively, and the source of latency is almost entirely the overhead of accessing the filesystem, with GC cleanup a distant second. The bash implementation (at least last I checked) is an absolute glutton with syscalls, caching almost nothing, walking directories multiple times, etc. And that's fine, because that's what it's for. It's built to be intuitive, consistent, and efficient with memory usage, not to be fast. There's even a note in man bash warning that it's slow if you do anything too complicated with it. (I commented out the bash tests from my glob benchmark, because they're too slow to be relevant. Like 100 to 10,000 times slower in many fairly straightforward cases. I only have them there to verify correctness of the returned results.)

Just to caveat this, of course, I haven't closely investigated bash's glob it in a few years, since a few years ago when bash 5.0 came out, but I doubt that much has changed, since the intended use case is still the same, and most potential improvements to performance would involve breaking changes in behavior. The original implementation of node-glob was a binding to Guido van Rossum's libglob in C, and I rewrote it in JS largely to make it go faster, as well as to add globstar, extglob, and brace expansion.

Also, as it seems @MoLow is finding, what we colloquially think of as "glob" is actually a few different things: posix regular expression classes, globstar, extended glob patterns, brace expansion, variable string expansion, and then file path portion expansion. If you pull in just that last part, but don't support any of the rest, it's going to be very underwhelming.

As I said previously:

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.

@isaacs Thank you very much for your detailed comment!

Also, as it seems @MoLow is finding, what we colloquially think of as "glob" is actually a few different things: POSIX regular expression classes, globstar, extended glob patterns, brace expansion, variable string expansion, and then file path portion expansion. If you pull in just that last part, but don't support any of the rest, it's going to be very underwhelming.

yes, I tried adding more tests from the minimatch repo and learned that this PR includes a very very small part of what I really needed and expected from glob

As I said previously:

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.

the reason why we want glob in the core is for features in core such as node --test <pattern> or node --watch <pattern>
this leaves us with the option of adding minimatch or glob as a dependency and not exposing it at all (partof what was done at #47486).

If you really want to have globbing built into core, the reasonable thing to do is to pull in either fast-glob or node-glob. They're both "big" complex modules that do a lot of stuff and have a bunch of optimizations and moving parts, around 5-10 kLoC each, but they're extremely well tested, complete, and performant. The choice depends ultimately on what kind of features/behavior/performance are desired. Writing a brand new clean-room implementation is an option, but probably the costliest option possible, and given that fast-glob and node-glob evolved to be so similar to one another just by following the path of optimization, it's reasonable to assume that you'll end up with a very similar implementation anyway. Skip the effort, get the good result.

Also: expanding globs to a set of filesystem entries should really live in fs, not path, since it's inherently a filesystem walk. But it would make sense for minimatch (or whatever you land on for doing that part of the job) live in path, since that's just a string processing operation with no filesystem involvement.

Along the way, you'll need a comprehensive file-system walking implementation. (Bash's glob does not have one. It caches nothing and just keeps recursing until done. It's the slowest possible way to tackle this problem, but also the most straightforward and memory efficient, and the approach node-glob used to use.) Fast-glob uses @nodelib/fs.walk for this, node-glob uses path-scurry. May as well expose that as an fs.walk() method, as that's another common thing people need, and there's perhaps room for some lowlevel optimizations that would make node-glob and fast-glob even faster if they could take advantage of it.

this leaves us with the option of adding minimatch or glob as a dependency and not exposing it at all (partof what was done at #47486).

Why not pull it in as a dep and also expose it? (Not challenging, genuinely curious.)

const { minimatch } = require('node:internal/lib/minimatch.js')
path.match = (pattern, pathLike) => minimatch.match(pattern, pathLike)

const { glob, globSync, globIterate, globIterateSync } = require('node:internal/lib/glob.js')
fs.glob = (pattern, options, cb) => {
  glob(pattern, options).then(res => cb(null, res), cb)
}
fs.globSync = globSync
fs.promises.glob = glob
// ...

Like, ok, it's a lot of code, but you'll end up writing more or less the same code anyway to implement it (or wish you had, if you ship bash's globs lol)

edit: Oh, I see, something to do with not using Primordials. I'm not sure what those are 😅

Why not pull it in as a dep and also expose it? (Not challenging, genuinely curious.)

there were concerns raised about that #47486 (comment)
just a few issues with exposing it is it won't use built-in node errors or primordials,
changing the API won't necessarily respect node semver policy, etc etc.

Oh, I see, something to do with not using Primordials. I'm not sure what those are 😅

@isaacs primordials are basically a frozen reference to js built-ins to avoid them being tamperd
https://github.com/nodejs/node/blob/c6dabe30832e40dab7fa35fd2ccd0135c74ca091/doc/contributing/primordials.md

yes, I tried adding more tests from the minimatch repo and learned that this PR includes a very very small part of what I really needed and expected from glob

Also, take a look at the benchmark scripts in https://github.com/isaacs/node-glob. I don't think a node core implementation needs to necessarily be faster than fast-glob or node-glob, but it should be within that range to not be a hazard if it's exposed at all. node:test's usage is likely to be small enough to never be a huge issue, but I would bet anything that the second it lands, people are going to be asking for it to be exposed for their programs as well.

Oh, I see, something to do with not using Primordials. I'm not sure what those are 😅

@isaacs primordials are basically a frozen reference to js built-ins to avoid them being tamperd https://github.com/nodejs/node/blob/c6dabe30832e40dab7fa35fd2ccd0135c74ca091/doc/contributing/primordials.md

Ah, nice. Yeah, porting a module as involved as glob (+path-scurry, +minimatch, etc.) would be quite a bit of work, and likely make it painful to pull future patches.

Maybe it'd be possible to "mimic" some of that hardening in the library itself (or in a standalone userland module), and then just swap out import { ... } from './primordials.js' with the equivalent import from node:lib/internal/per_context/primordials.js? If it doesn't impact performance, I'd be open to it.

Along the way, you'll need a comprehensive file-system walking implementation. (Bash's glob does not have one. It caches nothing and just keeps recursing until done. It's the slowest possible way to tackle this problem, but also the most straightforward and memory efficient, and the approach node-glob used to use.) Fast-glob uses @nodelib/fs.walk for this, node-glob uses path-scurry. May as well expose that as an fs.walk() method, as that's another common thing people need, and there's perhaps room for some lowlevel optimizations that would make node-glob and fast-glob even faster if they could take advantage of it.

@isaacs If I understand correctly there is an effort to do something like this: #41439

Oh, neat.

That really needs a streaming API (ideally not a Stream per se, but a low level "get the next thing" kinda like the opendir/scandir stuff) as well as a way to filter whether a directory gets descended into or not, or it won't be of much use for a glob walker. Eg: https://github.com/isaacs/node-glob/blob/main/src/walker.ts#L238-L309