FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: expose glob and globSync by MoLow · Pull Request #51912 · nodejs/node · GitHub

/ node Public

fs: expose glob and globSync - #51912

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
MoLow:expose-glob
Mar 3, 2024
ConversationCommits (3)ChecksFiles changed
Merged

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

Conversation

MoLow commented Feb 28, 2024

Copy link
Copy Markdown
Member

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

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 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 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 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 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 commented Feb 28, 2024
View reviewed changes
Comment thread lib/internal/fs/glob.js Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Feb 28, 2024
View reviewed changes
Comment thread lib/internal/fs/glob.js Outdated
pluris reviewed Feb 28, 2024
View reviewed changes
Comment thread doc/api/fs.md Outdated
pluris reviewed Feb 28, 2024
View reviewed changes
Comment thread doc/api/fs.md Outdated

MoLow commented Feb 29, 2024

Copy link
Copy Markdown
Member Author

CC @nodejs/fs

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm for experimental

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 Outdated
Comment thread lib/internal/fs/promises.js Outdated

aduh95 commented Mar 2, 2024

Copy link
Copy Markdown
Contributor

/cc @nodejs/fs

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

atlowChemi commented Mar 3, 2024

Copy link
Copy Markdown
Member

cc @nodejs/primordials

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2024
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 commented Mar 3, 2024

Copy link
Copy Markdown
Collaborator

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

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 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 mentioned this pull request Jun 22, 2024
Closed

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
deps: swap minimatch with picomatch
8d31f8d 
Refs: nodejs#51912 (comment)
fregante mentioned this pull request Jul 1, 2024
Closed
4 tasks
bizob2828 mentioned this pull request Jul 9, 2024
Closed
voxpelli mentioned this pull request Aug 22, 2024
Closed
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
fs: refactoring declaratively with the fromAsync function
07d075a 
- Refs: nodejs#51912
sheremet-va mentioned this pull request Sep 25, 2024
Open
6 tasks
aduh95 pushed a commit that referenced this pull request Sep 27, 2024
fs: refactoring declaratively with Array.fromAsync
090add7 
Refs: #51912
PR-URL: #54644
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
bcherny mentioned this pull request Sep 28, 2024
Closed
ebadiere mentioned this pull request Nov 11, 2024
Closed
8 tasks
karlhorky mentioned this pull request Jan 11, 2025
Open
robhogan mentioned this pull request Jul 18, 2025
Merged
atkei mentioned this pull request Nov 9, 2025
Closed
This was referenced Mar 17, 2026
Merged
Merged
coderabbitai Bot mentioned this pull request Apr 7, 2026
Merged
coderabbitai Bot mentioned this pull request May 18, 2026
Merged
coderabbitai Bot mentioned this pull request Jun 16, 2026
Merged
mattbutlerengineering mentioned this pull request Jul 12, 2026
Closed
15 tasks
coderabbitai Bot mentioned this pull request Jul 27, 2026
Merged
16 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
aduh95 aduh95 approved these changes
atlowChemi atlowChemi approved these changes
+3 more reviewers
pluris pluris left review comments
felixfbecker felixfbecker left review comments
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 outstanding review comments, 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.
Back | FazBrowse Home | New Git URL