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

[eslint-patch] Improve performance and correctness of eslint-bulk-suppressions #5055

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dmichon-msft
Copy link
Contributor

Summary

Fixes #5054
Fixes #5006

Fixes an issue with incorrect computation of scope ids that were resulting in overly broad suppressions.

Details

  • Simplifies the patching logic to load ESLint's linter.js into memory and only override Linter.prototype.verify, without generating any temporary files.
  • Uses AST traversal to find the relevant node instead of extracting it from the reporter, due to limitations of data visibility. However, the new version only processes reported problems that aren't already suppressed via inline comments.
  • Adds a new environment variable ESLINT_BULK_ESLINTRC_FOLDER_PATH that, if specified, short-circuits the patch searching for .eslintrc.js from every single file.
  • Updates @rushstack/heft-eslint-plugin to set ESLINT_BULK_ESLINTRC_FOLDER_PATH to the parent of its detected ESLint config.
  • Adds a tag eslint-tests to the ESLint test projects to make them easier to run
  • Updates the ESLint bulk suppressions test project to run the suppression tester as well

How it was tested

Ran in the ESLint test projects.
Linked this version of the patch into a repository that has tons of bulk suppressions and ran it there.
Profiled this version vs. the old, for the projects tested the latency was cut roughly in half, though the AST walk is now most of the cost.

Impacted documentation

Behavior should be the same, other than ensuring that the suppressions from this feature show up in the output of getSuppressedMessages().

if (!newSerializedSuppressions.has(serializedSuppression)) {
newSerializedSuppressions.add(serializedSuppression);
newSuppressions.push(suppression);
suppressions.push(suppression);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
suppressions.push(suppression);
if (!serializedSuppressions.has(suppression)) {
suppressions.push(suppression);
}


const highStack: number[] = [];

traverser.traverse(sourceCode.ast, {
Copy link
Contributor Author

@dmichon-msft dmichon-msft Dec 22, 2024

Choose a reason for hiding this comment

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

This is producing suboptimal results when the desired position is inside of a comment. Additional logic will need to be added to detect which node a comment is associated with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs triage
1 participant