feat(componentGroups): rename InvalidObjectProps to MissingPageProps #799
Conversation
| function formatDefaultMessage(oldName: string, newName: string) { | ||
| return `${oldName} has been renamed to ${newName}.`; | ||
| } |
There was a problem hiding this comment.
Not a blocker, but wondering if we should add a directory to helpers for these sort of default message functions. defaultMessages as the directory -> renameIdentifier.tsx as this helper for example. Or just yanking this out as its own individual helper to be reused elsewhere.
| !imports.some( | ||
| (specifier) => specifier.imported.name === node.imported.name |
There was a problem hiding this comment.
Could we either rework/rename the checkSpecifierExists function in the checkMatchingImportDeclaration file to be used here, or create a matcher helper specifically for a specifier only?
| if (node.typeName.type === "Identifier") { | ||
| replaceIdentifier(node.typeName); | ||
| } | ||
| }, | ||
| TSInterfaceHeritage(node: TSESTree.TSInterfaceHeritage) { | ||
| if (node.expression.type === "Identifier") { |
There was a problem hiding this comment.
Similar to above, maybe a matcher helper for these 2? I feel like rather than replaceIdentifier having a check to return early if an interface name isn't found in the imports, we should instead do what we're doing with the ImportSpecifier block and check first and only call the code to replace if a match is found.
There was a problem hiding this comment.
I updated this to have a separate function to check whether the renaming should be done.
Do you see a reason in extracting that to a separate helper as well?
| @@ -0,0 +1,18 @@ | |||
| ### component-groups-invalidObjectProps-rename-to-missingPageProps [(react-component-groups/#313)](https://github.com/patternfly/react-component-groups/pull/313) | |||
There was a problem hiding this comment.
This isn't a blocker for this PR, but sorta wondering if this should really be its own separate rule or included as part of the pre-existing one (with a rename maybe). Really the question is, "should there be a single rule dealing with anything InvalidObject rename related?"
Doing so would require a little refactoring both to the existing rule and the new helper here possibly, but also depends whether doing so would just bloat the existing rule too much. Having them separate may be more clear and understandable than trying to combine them, but curious what you think.
There was a problem hiding this comment.
"should there be a single rule dealing with anything InvalidObject rename related?"
I think ideally there should be just a single rule, but...
Doing so would require a little refactoring
I am afraid it could end up being "a lot of" refactoring, given that the current renameComponent helper already returns a function. But now that I am thinking about it, we could combine it and do something like:
create: (context: Rule.RuleContext) => {...renameComponent(args)(context), ...renameInterface(args)(context)}
But maybe it will not be that easy
| { | ||
| code: `import { InvalidObjectProps } from '@patternfly/react-core';`, | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| { | ||
| code: `import { InvalidObjectProps } from '@patternfly/react-component-groups'; |
There was a problem hiding this comment.
Maybe a valid test for importing something else from the correct package, and an invalid test that imports InvalidObjectProps and something else to show that only InvalidObjectProps gets touched/flagged?
Closes #786
Adds a helper to rename interface. I added the
@typescript-eslint/utilspackage for that reason to type the given typescript Nodes. However it does it not fit 100% with the currentRule.RuleContextwe are using, so I had to do thenode as unknown as Nodecasting.In the future we might migrate to use
@typescript-eslint/utilsinstead ofestree-jsxentirely, but it would require basically updating every rule and helpers, so I don't know if it is worth it.