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

Add **/node_modules to file_scan_exclusions #12156

Closed
wants to merge 1 commit into from

Conversation

tekacs
Copy link

@tekacs tekacs commented May 22, 2024

For new users on my team, the first thing that we have to change to get viable search performance is adding "**/node_modules" to file_scan_exclusions.

Apologies if this is the wrong way to solve this problem, but search is interminably slow and painful without this change, so I thought I'd try PR-ing this addition to see if there's appetite for including it by default!

Release Notes:

  • Improved the default file_scan_exclusions by including "**/node_modules", to improve search performance by default.

For new users on my team, the first thing that we have to change to get viable search performance is adding `"**/node_modules"` to `file_scan_exclusions`.

Apologies if this is the wrong way to solve this problem, but search is interminably slow and painful without this change, so I thought I'd try PR-ing this addition to see if there's appetite for including it by default!
Copy link

cla-bot bot commented May 22, 2024

We require contributors to sign our Contributor License Agreement, and we don't have @tekacs on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'.

@tekacs
Copy link
Author

tekacs commented May 22, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 22, 2024
Copy link

cla-bot bot commented May 22, 2024

The cla-bot has been summoned, and re-checked this pull request!

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

Thank you, but this does not look like a good solution indeed:

  • we have a project search feature, that allows to look search in ignored, but not excluded, files — merging this PR will break such searches
  • file finder is also requested to match ignored files, and that matching has to be fast, hence indexed somehow — excluded files are not indexed

Both features were used/required with node_modules specifically, based on the feedback.

What's more interesting, current implementation of Zed does not traverse ignored roots unless those are expanded — so interesting to know the details on how the performance degrades.

It would also be interesting to get things profiled and see what's slow actually.

If all above sounds too hard, the only good bandage aid on top I can think of is #7195 — then you would be able to store the exclusion in the repo, propagating it to every teammate automatically.

@SomeoneToIgnore SomeoneToIgnore self-assigned this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants