[eslint-plugin] feat: only lint MenuItem with popoverProps attr #5495
Conversation
[eslint-plugin] feat: only lint MenuItem with popoverProps attrPreviews: documentation | landing | table | demo |
packages/eslint-plugin/src/rules/no-deprecated-components/createNoDeprecatedComponentsRule.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| if (deprecatedProp !== undefined) { | ||
| const deprecatedComponentKey = Object.keys(deprecatedToNewComponentMapping).find( | ||
| key => key.split(".")[0] === node.name, |
There was a problem hiding this comment.
it is technically possible to have both MenuItem and MenuItem.popoverProps as keys, in which case this would return the wrong key, right?
| key => key.split(".")[0] === node.name, | |
| key => key.indexOf(".") >= 0 && key.split(".")[0] === node.name, |
There was a problem hiding this comment.
Yeah, I think it would fail in that case. The rule doesn't gracefully handle bad configuration right now, but I figure it's ok to be a little simplistic here since the configuration is not exposed to consumers -- it only lives inside the rule implementation in this package.
There was a problem hiding this comment.
fixed it up a little to guard against the edge case you mentioned
| const deprecatedProp = node.parent.attributes.find( | ||
| attribute => | ||
| attribute.type === TSESTree.AST_NODE_TYPES.JSXAttribute && | ||
| attribute.name.type === TSESTree.AST_NODE_TYPES.JSXIdentifier && | ||
| deprecatedToNewComponentMapping[`${node.property.name}.${attribute.name.name}`] != null, | ||
| ); | ||
| if (deprecatedProp !== undefined) { | ||
| const deprecatedComponentKey = Object.keys(deprecatedToNewComponentMapping).find( | ||
| key => key.split(".")[0] === node.property.name, | ||
| ); | ||
| context.report({ | ||
| data: { | ||
| deprecatedComponentName: node.property.name, | ||
| deprecatedPropName: ( | ||
| (deprecatedProp as TSESTree.JSXAttribute).name as TSESTree.JSXIdentifier | ||
| ).name, | ||
| newComponentName: deprecatedToNewComponentMapping[deprecatedComponentKey!], | ||
| }, | ||
| messageId: "migrationWithPropUsage", | ||
| node, | ||
| }); | ||
| } |
There was a problem hiding this comment.
is this code repeated, just for a different node type?
There was a problem hiding this comment.
it's a little different, it uses node.property.name instead of node.name. but yes it's very similar to the other block. I think it would be a little convoluted / unreadable if we tried to DRY this code between the two syntax handlers.
There was a problem hiding this comment.
on second thought, I've refactored this bit into a function shared between the two handlers, it's still legible
DRY rule implementation based on CR feedbackPreviews: documentation | landing | table | demo |
fix lint flagPreviews: documentation | landing | table | demo |
Merge remote-tracking branch 'origin/develop' into ad/eslint-menuitemPreviews: documentation | landing | table | demo |
fix bad mergePreviews: documentation | landing | table | demo |
minor rule documentation fixPreviews: documentation | landing | table | demo |
Checklist
Changes proposed in this pull request:
Improve
@blueprintjs/no-deprecated-core-components&@blueprintjs/no-deprecated-componentslint rule to understand components usage which is deprecated iff a specific prop is used. This is particularly useful for<MenuItem>, where we don't want to force consumers to unnecessarily migrate to<MenuItem2>in the overwhelming majority use case... we only need them to migrate whenpopoverPropsis used. This will greatly reduce the code diff required in migrations.Reviewers should focus on:
Implementation & test cases