NEW(eslint): @W-18495508@: Implement the "no-user-config" scenario for ESLint v9 #301
Conversation
| * @param paths It is assumed that paths is a non-empty array of absolute value paths. | ||
| */ | ||
| export function calculateLongestCommonParentFolderOf(paths: string[]): string | null { |
There was a problem hiding this comment.
Note that this was just moved (not modified in any way).
I had to share this utility with the eslint9 engine to help with the calculation of the base directory for ESLint 9. This meant a change to the engine-api package which meant all the engines in this monorepo needed to increment their dependency on it. Sorry for the noise.
| "@typescript-eslint/eslint-plugin": "^8.32.1", | ||
| "@typescript-eslint/parser": "^8.32.1", | ||
| "eslint": "^9.26.0", | ||
| "eslint-plugin-import": "^2.31.0", | ||
| "eslint-plugin-jest": "^28.11.0", | ||
| "tmp": "^0.2.3", |
There was a problem hiding this comment.
I look forward to the day I have time to do W-17875752 so that we can just have core pass in the temp folder so that we don't need to have the tmp dependency on all our engines.
| @@ -636,6 +624,10 @@ export const RULE_MAPPINGS: Record<string, {severity: SeverityLevel, tags: strin | |||
| severity: SeverityLevel.High, | |||
| tags: [/* NOT RECOMMENDED */ COMMON_TAGS.CATEGORIES.ERROR_PRONE, COMMON_TAGS.LANGUAGES.JAVASCRIPT] // Not available with TypeScript | |||
| }, | |||
| "no-useless-assignment": { | |||
There was a problem hiding this comment.
ESLint v9 has removed a few rules and added a few rules for base javascript and typescript. So the the updates to the gold files and rule-mappings reflect this.
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
5c71df0 to
e7b976a
Compare
| // TODO: We should make our DescribeRulesProgressEvents more refined while calculating the eslint context information | ||
| expect(describeRulesProgressEvents.map(e => e.percentComplete)).toEqual([0, 10, 90, 95, 100]); |
There was a problem hiding this comment.
This file was filled with the tests for the default (no user config) case...and it's really great that the only thing behavior change was the progress events (which I plan on enhancing at some point by the way) and gold files.
This proves that between v8 and v9 - I have successfully maintained the default behavior!
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
e7b976a to
7135d52
Compare
| ): object | null | string | number | boolean | ||
| { | ||
| const seen = new WeakMap<object, string>(); |
There was a problem hiding this comment.
Just to make sure I understand, is the WeakMap being used here so that entries added during recursive calls fall out as the recursion un-nests?
There was a problem hiding this comment.
I just know from experience that you need to use a WeakMap when doing infinite loop detection. I'll let chatGPT answer this one for me:
| return configs; | ||
| } | ||
|
|
||
| private useJsConfig(): boolean { |
There was a problem hiding this comment.
Could we maybe call these methods something like useBaseJsConfig etc for clarity's sake?
There was a problem hiding this comment.
Sure. Done.
| const userConfigInfo: UserConfigInfo = this.getUserConfigInfo(describeOptions.workspace); | ||
| this.emitLogEvent(LogLevel.Fine, `Detected the following regarding the user's eslint configuration files: ${userConfigInfo}`); | ||
| this.emitLogEvent(LogLevel.Fine, `Detected the following user's ESLint configuration state: ${userConfigInfo}`); |
There was a problem hiding this comment.
"Following user's configuration state" sounds like it's indicating who the configuration state belongs to, not that it's indicating the configuration state itself. Maybe "Following user-provided configuration state"?
There was a problem hiding this comment.
Sure I can fix the wording here.
Done.
| } | ||
|
|
||
| function toSeverityLevelForCustomRule(metadata: RulesMeta, status: ESLintRuleStatus): SeverityLevel { | ||
| if (status === ESLintRuleStatus.WARN) { |
There was a problem hiding this comment.
I understand why you made the choices you made, but it feels really weird to me that warn produces a lower-severity violation than layout. I feel like if warn is being turned to info, then so should layout.
There was a problem hiding this comment.
An ESLint warn is meant to not fail eslint in any way but to just inform the user about something. This is indeed a lower severity that layout errors.
I didn't change these - these came from our original eslint 8 engine design which we already had reviewed, etc. Keeping the rules the exact same as they were before.
| @@ -0,0 +1,37 @@ | |||
| // This declaration adds in the missing types for "@lwc/eslint-plugin-lwc-platform" whose package.json file's main field points to: | |||
| // node_modules/@lwc/eslint-plugin-lwc-platform/lib/index.js | |||
| declare module '@lwc/eslint-plugin-lwc-platform' { | |||
There was a problem hiding this comment.
Is this being declared on top of modules that already exist, or are we declaring these modules from whole cloth?
There was a problem hiding this comment.
See https://salesforce-internal.slack.com/archives/C5W3E40TC/p1746136385115359
We depend on those packages but they have no types. Until the other team adds in types for typescript, I created the types for them here locally with our own .d.ts file.
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
7135d52 to
ab9386b
Compare
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
ab9386b to
fffc2d1
Compare
No description provided.