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: add support for minimatch overrides #52789

Closed
wants to merge 2 commits into from

Conversation

RedYetiDev
Copy link
Member

This PR adds an additional overrides parameter to the options argument of fs.glob. This allows users to override the default minimatch glob options.

Fixes #52779

@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 May 1, 2024
lib/internal/fs/glob.js Outdated Show resolved Hide resolved
@@ -200,6 +202,7 @@ class Glob {
optimizationLevel: 2,
platform: process.platform,
nocaseMagicOnly: true,
...overrides,
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test?

Co-authored-by: Yagiz Nizipli <[email protected]>
@@ -1082,6 +1082,8 @@ added: v22.0.0
* `cwd` {string} current working directory. **Default:** `process.cwd()`
* `exclude` {Function} Function to filter out files/directories. Return
`true` to exclude the item, `false` to include it. **Default:** `undefined`.
* `overrides` {Object} override the default `minimatch` behavior with
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinion only, I don't think Node.js should expose API like this.
It means this API will always be tie to minimatch and will becomes a resistance on enhancement or provide node vendor-ed one.

@tniessen
Copy link
Member

tniessen commented May 2, 2024

If folks want to expose minimatch's API details to Node.js users, why is there fs.glob() with a different API in the first place?

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

minimatch options are not necessarily bound to our semver release cycle, and are not tested individually.
if we want to support additional options, we should add them one by one - with individual tests and documentation for each of these options

@MoLow
Copy link
Member

MoLow commented May 2, 2024

I also am not sure this is the ask in #52779 - see #52779 (comment)

@climba03003
Copy link
Contributor

I believe it more like to ask support for glob string or array in the exclude option.

@RedYetiDev
Copy link
Member Author

Yes, that sounds like a better idea.

@RedYetiDev RedYetiDev closed this May 2, 2024
@RedYetiDev RedYetiDev deleted the fs-minimatch branch May 2, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacement for minimatch
6 participants