NEW(eslint): @W-18669872@: Enhance progress events for eslint v9 engine #306
Conversation
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
59215f9 to
44c8672
Compare
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
44c8672 to
2d4ecb2
Compare
| @@ -104,7 +104,6 @@ export class BaseConfigFactory { | |||
| configs.push({ | |||
| ...conf, | |||
| files: this.engineConfig.file_extensions.typescript.map(ext => `**/*${ext}`), | |||
| ignores: this.engineConfig.file_extensions.javascript.map(ext => `**/*${ext}`), // TODO: Confirm whether this works or not | |||
There was a problem hiding this comment.
Yeah turns out this wasn't needed. :-)
| if (specifiedRules.has(violation.ruleName)) { | ||
| violations.push(violation); | ||
| } else { | ||
| // This may be possible if a user tries to suppress an eslint rule in their code that isn't available. We just ignore it but debug it just in case. | ||
| this.emitLogEvent(LogLevel.Debug, getMessage('ViolationFoundFromUnregisteredRule', violation.ruleName, JSON.stringify(violation,null,2))) | ||
| } |
There was a problem hiding this comment.
Fix as you go item: This code was with the eslint 8 engine... I didn't think we would need it with eslint 9's ruleFilter but as I did my full gambit of tests... it turns out we still do.
| ViolationFoundFromUnregisteredRule: | ||
| `A rule with name '%s' produced a violation, but this rule was not registered with the 'eslint' engine so it will not be included in the results.\n` + | ||
| `This may occur if in your file you are using inline comments to attempt to disable or configure this rule even though it is unknown to ESLint and Code Analyzer.\n` + | ||
| `Ignored Violation:\n%s`, |
There was a problem hiding this comment.
This already exististed with eslint 8... just pulled it into eslint 9... so no need for UI text review here.
| // ESLint doesn't natively know about various browser and node globals. So we add them here to | ||
| // remove false positives for our users. | ||
| ... globals.node, | ||
| ... globals.browser, | ||
| ... globals.es2017 |
There was a problem hiding this comment.
Fix as you go item in response to seeing noise when scanning a ton of repos. This should make users happy :-)
| expect(logEvents).toContainEqual({ | ||
| type: EventType.LogEvent, | ||
| logLevel: LogLevel.Debug, | ||
| message: getMessage('ViolationFoundFromUnregisteredRule', 'oops-rule', JSON.stringify(filteredViolation, null, 2)) |
There was a problem hiding this comment.
What is our rationale for logging these violations instead of just including them with the results? I can't remember, and now that I'm seeing it again it looks weird to me.
There was a problem hiding this comment.
Good question. We do the same thing with eslint 8. Basically the rule name associated with the violation that eslint produces isn't one of the rules selected.
Basically ESLint 8 and 9 both create a proper violation that ends up in our official engine results whenever it wants to complain that a directive is used for an unknown rule. But Core complains when a violation associated with a rule that wasn't selected show up. This makes the engine fail completely since it is a post engine validation check - which we don't want.
Ideally ESLint 8 and 9 should just emit a warning. It's odd that it adds this as a violation. This is even more odd given that the ruleFilter doesn't include this rule. So ESLint is kind of violating their own contract in reporting a violation for a rule that wasn't in the ruleFilter gate.
There was a problem hiding this comment.
Okay, yeah, the core requirement that all violations be for rules that actually exist does totally justify the current behavior.
No description provided.