Skip to content

fs: expose glob and globSync#51912

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
MoLow:expose-glob
Mar 3, 2024
Merged

fs: expose glob and globSync#51912
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
MoLow:expose-glob

Conversation

@MoLow

@MoLow MoLow commented Feb 28, 2024

Copy link
Copy Markdown
Member

Fixes #40731
This PR introduces fs.globand fsPromises.glob.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Feb 28, 2024
@MoLow

MoLow commented Feb 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

this capability was originally added to core for the use of the test runner, and we did not expose it since some dependencies don't use primordials.
there have been recent discussions regarding the primordials and whether or not they are a must for 100% of core code.
My personal opinion is primordials should be used to prevent catastrophic failures and such an addition to core has more benefits than drawbacks.
this will also not be precedential - we have some dependencies that don't use primordials, such as undici
we might want to add tsc-agenda Issues and PRs to discuss during the meetings of the TSC. to discuss this

@MoLow MoLow added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Feb 28, 2024
@github-actions

github-actions Bot commented Feb 28, 2024

Copy link
Copy Markdown
Contributor

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. Otherwise, the commit will be placed in the Other Notable Changes section.

@MoLow MoLow linked an issue Feb 28, 2024 that may be closed by this pull request
[node:test] Support for file path patterns in run() #48609
Closed
MoLow
MoLow commented Feb 28, 2024
View reviewed changes
Comment thread lib/internal/fs/glob.js
}


async* glob() {

@MoLow MoLow Feb 28, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty much the same implementation as globSync except using yield instead if this.#results.push

@benjamingr benjamingr Mar 1, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementations can be unified probably if you yield where you await and then do sync/async I/O depending on the path not sure if it's worth it

MoLow
MoLow commented Feb 28, 2024
View reviewed changes
Comment thread lib/internal/fs/glob.js Outdated
pluris
pluris reviewed Feb 28, 2024
View reviewed changes
Comment thread doc/api/fs.md Outdated
pluris
pluris reviewed Feb 28, 2024
View reviewed changes
Comment thread doc/api/fs.md Outdated
@MoLow

MoLow commented Feb 29, 2024

Copy link
Copy Markdown
Member Author

CC @nodejs/fs

benjamingr
benjamingr reviewed Mar 1, 2024
View reviewed changes
Comment thread doc/api/fs.md
benjamingr
benjamingr reviewed Mar 1, 2024
View reviewed changes
Comment thread doc/api/fs.md Outdated
benjamingr
benjamingr reviewed Mar 1, 2024
View reviewed changes
Comment thread lib/internal/fs/glob.js
benjamingr
benjamingr approved these changes Mar 1, 2024
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm for experimental

aduh95
aduh95 reviewed Mar 2, 2024
View reviewed changes
Comment thread lib/internal/fs/glob.js Outdated
Comment thread lib/internal/fs/glob.js Outdated
Comment thread lib/internal/fs/glob.js Outdated
Comment thread lib/internal/fs/glob.js
Comment thread lib/internal/fs/promises.js Outdated
@aduh95

aduh95 commented Mar 2, 2024

Copy link
Copy Markdown
Contributor

/cc @nodejs/fs

@MoLow MoLow requested review from aduh95 and benjamingr March 3, 2024 09:24
@atlowChemi

atlowChemi commented Mar 3, 2024

Copy link
Copy Markdown
Member

cc @nodejs/primordials

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 3, 2024

Copy link
Copy Markdown
Collaborator

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

@marco-ippolito marco-ippolito added baking-for-lts PRs that need to wait before landing in a LTS release. and removed backport-requested-v20.x labels May 2, 2024
@styfle

styfle commented May 4, 2024

Copy link
Copy Markdown
Member

@jonschlinkert It might be worth creating a new issue since this one is closed

@marco-ippolito marco-ippolito added backport-requested-v20.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Jun 17, 2024
danielbayley added a commit to danielbayley/node that referenced this pull request Jun 22, 2024
@danielbayley
deps: swap minimatch with picomatch
add50c9 
Refs: nodejs#51912 (comment)
danielbayley added a commit to danielbayley/node that referenced this pull request Jun 22, 2024
@danielbayley
deps: swap minimatch with picomatch
035846c 
Refs: nodejs#51912 (comment)
@danielbayley danielbayley mentioned this pull request Jun 22, 2024
Closed
@danielbayley

danielbayley commented Jun 22, 2024

Copy link
Copy Markdown
Contributor

I see that his is actually depending on minimatch, I would have preferred to see picomatch or micromatch given that minimatch is subject to catastrophic backtracking, and both picomatch and micromatch are more performant, support more patterns, and are better tested. Picomatch also has a tokenizer that can be used. Is there a reason minimatch is being used or can we do a PR to swap in micromatch?

Here are the benchmarks.

I know this isn't the issue for this, I'd be happy to open a new one.

@jonschlinkert It might be worth creating a new issue since this one is closed

@jonschlinkert @styfle See: #53547.

danielbayley added a commit to danielbayley/node that referenced this pull request Jun 23, 2024
@danielbayley
deps: swap minimatch with picomatch
8d31f8d 
Refs: nodejs#51912 (comment)
@fregante fregante mentioned this pull request Jul 1, 2024
Closed
4 tasks
@bizob2828 bizob2828 mentioned this pull request Jul 9, 2024
Closed
@voxpelli voxpelli mentioned this pull request Aug 22, 2024
Closed
@kamilmysliwiec kamilmysliwiec mentioned this pull request Aug 26, 2024
Closed
1 task
sonsurim added a commit to sonsurim/node that referenced this pull request Aug 29, 2024
@sonsurim
fs: refactoring declaratively with the fromAsync function
4dd3a9e 
Refs: nodejs#51912
sonsurim added a commit to sonsurim/node that referenced this pull request Aug 29, 2024
@sonsurim
fs: refactoring declaratively with the fromAsync function
07d075a 
- Refs: nodejs#51912
@sheremet-va sheremet-va mentioned this pull request Sep 25, 2024
Open
6 tasks
aduh95 pushed a commit that referenced this pull request Sep 27, 2024
@sonsurim
fs: refactoring declaratively with Array.fromAsync
090add7 
Refs: #51912
PR-URL: #54644
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
@bcherny bcherny mentioned this pull request Sep 28, 2024
Closed
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@sonsurim @louwers
fs: refactoring declaratively with Array.fromAsync
055c0ae 
Refs: nodejs#51912
PR-URL: nodejs#54644
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
@ebadiere ebadiere mentioned this pull request Nov 11, 2024
Closed
8 tasks
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@sonsurim @tpoisseau
fs: refactoring declaratively with Array.fromAsync
bb9e84a 
Refs: nodejs#51912
PR-URL: nodejs#54644
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
@karlhorky karlhorky mentioned this pull request Jan 11, 2025
Open
@robhogan robhogan mentioned this pull request Jul 18, 2025
Merged
@atkei atkei mentioned this pull request Nov 9, 2025
Closed
This was referenced Mar 17, 2026
Merged
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Apr 7, 2026
Merged
@coderabbitai coderabbitai Bot mentioned this pull request May 18, 2026
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jun 16, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@aduh95 aduh95 aduh95 approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

+3 more reviewers

@pluris pluris pluris left review comments

@felixfbecker felixfbecker felixfbecker left review comments

@Linkgoron Linkgoron Linkgoron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[node:test] Support for file path patterns in run() fs/path: expose internal-api minimatch/glob under path module.