chore: convert ESLint files from .js/.mjs to .ts#109310
chore: convert ESLint files from .js/.mjs to .ts#109310JoshuaKGoldberg merged 1 commit intomasterfrom
Conversation
45c26ec to
6dc8082
Compare
6dc8082 to
21c9406
Compare
21c9406 to
877d1ae
Compare
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: `Disallow imports from "${FORBIDDEN_PATH}" and autofix to "${REPLACEMENT_PATH}".`, | ||
| recommended: true, |
There was a problem hiding this comment.
This was typescript-eslint/typescript-eslint#8695 -> typescript-eslint/typescript-eslint#9025. The only "required" field for meta.docs is description, and arguably even that isn't really used anywhere. Requiring docs.recommended in earlier versions of typescript-eslint was a types bug.
|
|
||
| create(context) { | ||
| defaultOptions: [{}], | ||
| create(context, [options = {}]) { |
There was a problem hiding this comment.
typescript-eslint's RuleCreator has a nice little benefit of adding in some options defaulting & merging. https://typescript-eslint.io/developers/custom-rules/#handling-rule-options
| */ | ||
| export function shouldAnalyze(context) { | ||
| export function shouldAnalyze(context: TSESLint.RuleContext<string, unknown[]>) { | ||
| const sourceCode = context.sourceCode ?? context.getSourceCode(); |
There was a problem hiding this comment.
[Question] context.getSourceCode() is deprecated. context.sourceCode always exists in the one ESLint version we use. I don't know of a need for a fallback. Was this kept in for a specific reason?
There was a problem hiding this comment.
i doubt it. let’s trust the types and remove it 👍
|
Converting to draft while I address cursor's feedback. Edit: and TkDodo's |
| */ | ||
| export function shouldAnalyze(context) { | ||
| export function shouldAnalyze(context: TSESLint.RuleContext<string, unknown[]>) { | ||
| const sourceCode = context.sourceCode ?? context.getSourceCode(); |
There was a problem hiding this comment.
i doubt it. let’s trust the types and remove it 👍
This comment was marked as resolved.
This comment was marked as resolved.
…ken's (#109326) Similar to #109310, changes the test extensions from `*.spec.mjs` to `*.spec.js`. The `m` is unnecessary because Jest is configured to treat their syntax as ESM either way. `static/eslint/eslintPluginScraps/src/rules/use-semantic-token.spec.js` had some failures in the helper-function-generated invalid cases: * Most were using the `invalidProperty` message ID instead of `invalidPropertyWithSuggestion`, and missing `data.suggestedCategory` * A few were actually not invalid at all and had zero errors
I noticed while working on #109310 that the latest version of [`@typescript-eslint/prefer-optional-chain`](https://typescript-eslint.io/rules/prefer-optional-chain) wasn't being used. Some code snippets like this weren't being reported: ```ts if (apple && apple.banana === "cherry") { ``` That was fixed by typescript-eslint/typescript-eslint#11533. This PR bumps typescript-eslint to the latest version and fixes new lint rule reports. It was mostly prefer-optional-chain reports; others are noted inline.
c1efe05 to
546749c
Compare
| function getDisplayName(nameNode: TSESTree.JSXTagNameExpression) { | ||
| if (nameNode.type === 'JSXMemberExpression') { | ||
| return `${nameNode.object.name}.${nameNode.property.name}`; | ||
| return `${(nameNode.object as TSESTree.JSXIdentifier).name}.${nameNode.property.name}`; |
There was a problem hiding this comment.
nameNode.object as TSESTree.JSXIdentifier
This might be a potential bug in the rule: nameNode.object is typed as type JSXTagNameExpression = JSXIdentifier | JSXMemberExpression | JSXNamespacedName;. JSXMemberExpression doesn't have a .name; JSXNamespacedName's .name is a JSXIdentifier rather than the expected string.
There was a problem hiding this comment.
Sounds legit—are you able to update this to handle the actual type more defensively?
There was a problem hiding this comment.
Done!
I used the fancy AST_NODE_TYPES enum from typescript-eslint too.
6a4f9b5 to
beaab7c
Compare
0772c3e to
b588726
Compare
b588726 to
8d06a5b
Compare
| if (slotState.has(propName)) { | ||
| throw new TypeError( | ||
| `Duplicate prop configuration for: ${propName} in slot ${slot.componentNames.join(', ')}` | ||
| `Duplicate prop configuration for: ${propName} in slot ${Array.from(state.componentNames).join(', ')}` |
There was a problem hiding this comment.
TypeScript pointed out that slot.componentNames is optional. Was this meant to be state.componentNames?
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
static/eslint/eslintPluginScraps/src/rules/restrict-jsx-slot-children.ts
Show resolved
Hide resolved
natemoo-re
left a comment
There was a problem hiding this comment.
Just a few notes that would be good to address before merge, but not worth blocking! Great work on this 🙌
| function getDisplayName(nameNode: TSESTree.JSXTagNameExpression) { | ||
| if (nameNode.type === 'JSXMemberExpression') { | ||
| return `${nameNode.object.name}.${nameNode.property.name}`; | ||
| return `${(nameNode.object as TSESTree.JSXIdentifier).name}.${nameNode.property.name}`; |
There was a problem hiding this comment.
Sounds legit—are you able to update this to handle the actual type more defensively?
| elementName = nameNode.name; | ||
| } else if (nameNode.type === 'JSXMemberExpression') { | ||
| elementName = `${nameNode.object.name}.${nameNode.property.name}`; | ||
| elementName = `${(nameNode.object as TSESTree.JSXIdentifier).name}.${nameNode.property.name}`; |
| context.report({ | ||
| node: translationArg, | ||
| message: 'td() must use ATTRIBUTE_METADATA[key].brief from @sentry/conventions', | ||
| messageId: 'forbidden', |
There was a problem hiding this comment.
Did we lose the message here?
There was a problem hiding this comment.
No, this message duplicates the forbidden message ID:
message is an old legacy ESLint property that isn't even in the TSESLint types.
…9725) Followup to #109310: modifies the logic in `restrict-jsx-slot-children` to: * Behavior: handle namespaces and property accesses for JSX tags * Internal: unified `getDisplayName` and the ad hoc stringification in the `JSXAttribute` visitor I split this out into its own PR because the way I wrote `getDisplayName` triggered some reports from ESLint that I wanted to ask about. Fixes https://linear.app/getsentry/issue/ENG-6933/followup-refactor-restrict-jsx-slot-childrens-getdisplayname
Directly changes files from
.mjsto.mts. I would have preferred the cleaner.tsbut that will require adding"type": "module"topackage.json, which is more work (which would be a great followup!)..ts, building on #109319's switching the package to be ESM by default. Imports are now TypeScript-style extension-less ones.Now that the plugin's files are typed, this applies a few types cleanups:
RuleCreatorto create rules, which gives nicer typings than the built-in ESLintRuleestree(only vanilla JS types) to@typescript-eslint/estree(full TS node types)This PR should have no changes to the lint rules' behavior. The only runtime code changes are for deleting unused parameters.
Fixes https://linear.app/getsentry/issue/ENG-6930/convert-eslint-files-from-jsmjs-to-ts-and-add-types