[eslint-patch] Improve performance of eslint-bulk-suppressions #5055
Conversation
dmichon-msft
requested review from
iclanton,
octogonz,
apostolisms and
D4N14L
as code owners
eslint/eslint-patch/src/eslint-bulk-suppressions/bulk-suppressions-patch.ts
Outdated
Show resolved
Hide resolved
eslint/eslint-patch/src/eslint-bulk-suppressions/bulk-suppressions-patch.ts
Outdated
Show resolved
Hide resolved
|
On closer inspection, this isn't feasible for legacy function-style rules and would be a pain to maintain. Ideally we should push for ESLint to natively support an |
Defer detection of bulk suppressions until after inline suppressions. Simplify patching.
dmichon-msft
force-pushed
the
eslint-bulk-overhaul
branch
from
ba07386 to
4534eaf
Compare
dmichon-msft
changed the title
|
@kevin-y-ang FYI |
| fix: fixFn | ||
| fix: fixFn, | ||
| // If requested, disable the scan for .eslintrc files relative to linted files | ||
| useEslintrc: !disableLintConfigSearch |
There was a problem hiding this comment.
I don't understand this option. According to the ESLint docs, it seems to completely suppress configuration:
To Address: You should be sure to use a configuration whenever you run ESLint. However, you can still run ESLint without a configuration by passing the
--no-eslintrcoption on the command line or setting theuseEslintrcoption tofalseforCLIEngine.
...but then where does the configuration come from? Is the idea that Heft itself locates .eslintrc.js and then provides its path via overrideConfigFile?
If so, then does Heft actually support multiple other files (probed using useEslintrc)? How would that interact with overrideConfigFile?
Without a test project verifying/illustrating how this would work, it might be better to set useEslintrc=false by default (or hardwire it to always be false). My concern is that quite a lot of people may upgrade Heft without realizing that they're supposed to update their rigs to set disableLintConfigSearch=true, and then they'll permanently be invoking heft-lint-plugin in a nonstandard configuration.
| "lint": { | ||
| "taskPlugin": { | ||
| "options": { | ||
| "disableLintConfigSearch": true |
There was a problem hiding this comment.
Why isn't the official rig updated to add this setting? (In a separate comment, I'm suggesting to eliminate this entirely.)
Or at the very least, we need some clear documentation explaining when someone should choose "disableLintConfigSearch": true vs false.
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'slinter.jsinto memory and only overrideLinter.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.ESLINT_BULK_ESLINTRC_FOLDER_PATHthat, if specified, short-circuits the patch searching for.eslintrc.jsfrom every single file.@rushstack/heft-eslint-pluginto setESLINT_BULK_ESLINTRC_FOLDER_PATHto the parent of its detected ESLint config.eslint-teststo the ESLint test projects to make them easier to rungetSuppressedMessages()output of the linter, for use by auditing.Edited:
"disableLintConfigSearch"to the options forheft-lint-pluginthat, if configured, prevents the linter from searching for config files based on the location of linted files and tells it to just use the config in the project root.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 to ensure no regression.
Impacted documentation
Behavior should be the same, other than ensuring that the suppressions from this feature show up in the output of
getSuppressedMessages().