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

Pass deep option to ignore filter to avoid unnecessary recursion #251

Merged
merged 2 commits into from
Jun 18, 2023

Conversation

fwouts
Copy link
Contributor

@fwouts fwouts commented Jun 15, 2023

When globbing through a very large directory, it's useful to use the deep option from fast-glob to stop at a maximum level of depth: https://github.com/mrmlnc/fast-glob#deep

However when using gitignore: true, the lookup for ignored files doesn't currently pass the deep option, meaning that we still go through every level of the directory.

This PR forwards the deep option to make it work as expected.

ignore.js Outdated
@@ -59,12 +59,13 @@ const getIsIgnoredPredicate = (files, cwd) => {
const normalizeOptions = (options = {}) => ({
cwd: toPath(options.cwd) || process.cwd(),
suppressErrors: Boolean(options.suppressErrors),
deep: options.deep ? Number.parseInt(options.deep, 10) : Number.POSITIVE_INFINITY,
Copy link
Owner

Choose a reason for hiding this comment

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

The Number.parseInt call seems wrong. The deep option is documented to be a number. If anything, we should throw if it's not a number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tend to agree. What would you think about a simple typeof check defaulting to Number.POSITIVE_INFINITY, without throwing? That way, fast-glob remains in charge of validating its own options.

Happy to implement the throwing too, maybe using https://www.npmjs.com/package/@sindresorhus/is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants