feat(componentGroups): rename InvalidObjectProps to MissingPageProps #799
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { Rule } from "eslint"; | ||
| import { TSESTree } from "@typescript-eslint/utils"; | ||
| import { Identifier, ImportSpecifier, Node } from "estree-jsx"; | ||
| import { getFromPackage } from "./getFromPackage"; | ||
|
|
||
| interface InterfaceRenames { | ||
| [currentName: string]: string; | ||
| } | ||
|
|
||
| function formatDefaultMessage(oldName: string, newName: string) { | ||
| return `${oldName} has been renamed to ${newName}.`; | ||
| } | ||
|
|
||
| export function renameInterface( | ||
| renames: InterfaceRenames, | ||
| packageName = "@patternfly/react-core" | ||
| ) { | ||
| return function (context: Rule.RuleContext) { | ||
| const oldNames = Object.keys(renames); | ||
| const { imports } = getFromPackage(context, packageName, oldNames); | ||
|
|
||
| if (imports.length === 0) { | ||
| return {}; | ||
| } | ||
|
|
||
| const replaceIdentifier = (identifier: Identifier) => { | ||
| const getMatchingImport = (name: string) => | ||
| imports.find((specifier) => specifier.local.name === name); | ||
|
|
||
| const matchingImport = getMatchingImport(identifier.name); | ||
| const shouldRename = | ||
| matchingImport && | ||
| matchingImport.local.name === matchingImport.imported.name; | ||
|
|
||
| if (!shouldRename) { | ||
| return; | ||
| } | ||
|
|
||
| const oldName = identifier.name; | ||
| const newName = renames[oldName]; | ||
|
|
||
| context.report({ | ||
| node: identifier, | ||
| message: formatDefaultMessage(oldName, newName), | ||
| fix(fixer) { | ||
| return fixer.replaceText(identifier, newName); | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| return { | ||
| ImportSpecifier(node: ImportSpecifier) { | ||
| if ( | ||
| !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? |
||
| ) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const oldName = node.imported.name; | ||
| const newName = renames[oldName]; | ||
|
|
||
| context.report({ | ||
| node, | ||
| message: formatDefaultMessage(oldName, newName), | ||
| fix(fixer) { | ||
| return fixer.replaceText(node.imported, newName); | ||
| }, | ||
| }); | ||
| }, | ||
| TSTypeReference(node: TSESTree.TSTypeReference) { | ||
| 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? |
||
| replaceIdentifier(node.expression); | ||
| } | ||
| }, | ||
| }; | ||
| }; | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.
I think ideally there should be just a single rule, but...
I am afraid it could end up being "a lot of" refactoring, given that the current But maybe it will not be that easy |
||
|
|
||
| In react-component-groups, we've renamed InvalidObjectProps interface to MissingPageProps | ||
|
|
||
| #### Examples | ||
|
|
||
| In: | ||
|
|
||
| ```jsx | ||
| %inputExample% | ||
| ``` | ||
|
|
||
| Out: | ||
|
|
||
| ```jsx | ||
| %outputExample% | ||
| ``` | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| const ruleTester = require("../../ruletester"); | ||
| import * as rule from "./component-groups-invalidObjectProps-rename-to-missingPageProps"; | ||
|
|
||
| const message = `InvalidObjectProps has been renamed to MissingPageProps.`; | ||
|
|
||
| ruleTester.run( | ||
| "component-groups-invalidObjectProps-rename-to-missingPageProps", | ||
| rule, | ||
| { | ||
| valid: [ | ||
| // missing import | ||
| { | ||
| code: `const props: InvalidObjectProps;`, | ||
| }, | ||
| // import from wrong package | ||
| { | ||
| 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? |
||
| const props: InvalidObjectProps; | ||
| const otherProps = props as InvalidObjectProps; | ||
| interface CustomProps extends InvalidObjectProps {};`, | ||
| output: `import { MissingPageProps } from '@patternfly/react-component-groups'; | ||
| const props: MissingPageProps; | ||
| const otherProps = props as MissingPageProps; | ||
| interface CustomProps extends MissingPageProps {};`, | ||
| errors: [ | ||
| { | ||
| message, | ||
| type: "ImportSpecifier", | ||
| }, | ||
| { | ||
| message, | ||
| type: "Identifier", | ||
| }, | ||
| { | ||
| message, | ||
| type: "Identifier", | ||
| }, | ||
| { | ||
| message, | ||
| type: "Identifier", | ||
| }, | ||
| ], | ||
| }, | ||
| // named import with alias | ||
| { | ||
| code: `import { InvalidObjectProps as InvObjProps } from '@patternfly/react-component-groups'; | ||
| const props: InvObjProps;`, | ||
| output: `import { MissingPageProps as InvObjProps } from '@patternfly/react-component-groups'; | ||
| const props: InvObjProps;`, | ||
| errors: [ | ||
| { | ||
| message, | ||
| type: "ImportSpecifier", | ||
| }, | ||
| ], | ||
| }, | ||
| // imports from dist | ||
| { | ||
| code: `import { InvalidObjectProps } from '@patternfly/react-component-groups/dist/cjs/InvalidObject';`, | ||
| output: `import { MissingPageProps } from '@patternfly/react-component-groups/dist/cjs/InvalidObject';`, | ||
| errors: [ | ||
| { | ||
| message, | ||
| type: "ImportSpecifier", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: `import { InvalidObjectProps } from '@patternfly/react-component-groups/dist/esm/InvalidObject';`, | ||
| output: `import { MissingPageProps } from '@patternfly/react-component-groups/dist/esm/InvalidObject';`, | ||
| errors: [ | ||
| { | ||
| message, | ||
| type: "ImportSpecifier", | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: `import { InvalidObjectProps } from '@patternfly/react-component-groups/dist/dynamic/InvalidObject';`, | ||
| output: `import { MissingPageProps } from '@patternfly/react-component-groups/dist/dynamic/InvalidObject';`, | ||
| errors: [ | ||
| { | ||
| message, | ||
| type: "ImportSpecifier", | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| } | ||
| ); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { renameInterface } from "../../helpers"; | ||
|
|
||
| // https://github.com/patternfly/react-component-groups/pull/313 | ||
| module.exports = { | ||
| meta: { fixable: "code" }, | ||
| create: renameInterface( | ||
| { | ||
| InvalidObjectProps: "MissingPageProps", | ||
| }, | ||
| "@patternfly/react-component-groups" | ||
| ), | ||
| }; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { InvalidObjectProps } from "@patternfly/react-component-groups"; | ||
|
|
||
| const props: InvalidObjectProps; | ||
| interface CustomProps extends InvalidObjectProps {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { MissingPageProps } from "@patternfly/react-component-groups"; | ||
|
|
||
| const props: MissingPageProps; | ||
| interface CustomProps extends MissingPageProps {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,13 @@ | |
| dependencies: | ||
| eslint-visitor-keys "^3.3.0" | ||
|
|
||
| "@eslint-community/eslint-utils@^4.4.0": | ||
| version "4.4.1" | ||
| resolved "https://registry.yarnpkg.com/@eslint-community/eslint-utils/-/eslint-utils-4.4.1.tgz#d1145bf2c20132d6400495d6df4bf59362fd9d56" | ||
| integrity sha512-s3O3waFUrMV8P/XaF/+ZTp1X9XBZW1a4B97ZnjQF2KYWaFD2A8KyFBsrsfSjEmjn3RGWAIuvlneuZm3CUK3jbA== | ||
| dependencies: | ||
| eslint-visitor-keys "^3.4.3" | ||
|
|
||
| "@eslint-community/regexpp@^4.4.0": | ||
| version "4.5.0" | ||
| resolved "https://registry.yarnpkg.com/@eslint-community/regexpp/-/regexpp-4.5.0.tgz#f6f729b02feee2c749f57e334b7a1b5f40a81724" | ||
|
|
@@ -1169,11 +1176,24 @@ | |
| "@typescript-eslint/types" "7.3.1" | ||
| "@typescript-eslint/visitor-keys" "7.3.1" | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "8.12.2" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/scope-manager/-/scope-manager-8.12.2.tgz#6db0213745e6392c8e90fe9af5915e6da32eb94a" | ||
| integrity sha512-gPLpLtrj9aMHOvxJkSbDBmbRuYdtiEbnvO25bCMza3DhMjTQw0u7Y1M+YR5JPbMsXXnSPuCf5hfq0nEkQDL/JQ== | ||
| dependencies: | ||
| "@typescript-eslint/types" "8.12.2" | ||
| "@typescript-eslint/visitor-keys" "8.12.2" | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "7.3.1" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/types/-/types-7.3.1.tgz#ae104de8efa4227a462c0874d856602c5994413c" | ||
| integrity sha512-2tUf3uWggBDl4S4183nivWQ2HqceOZh1U4hhu4p1tPiIJoRRXrab7Y+Y0p+dozYwZVvLPRI6r5wKe9kToF9FIw== | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "8.12.2" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/types/-/types-8.12.2.tgz#8d70098c0e90442495b53d0296acdca6d0f3f73c" | ||
| integrity sha512-VwDwMF1SZ7wPBUZwmMdnDJ6sIFk4K4s+ALKLP6aIQsISkPv8jhiw65sAK6SuWODN/ix+m+HgbYDkH+zLjrzvOA== | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "7.3.1" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/typescript-estree/-/typescript-estree-7.3.1.tgz#598848195fad34c7aa73f548bd00a4d4e5f5e2bb" | ||
|
|
@@ -1188,6 +1208,30 @@ | |
| semver "^7.5.4" | ||
| ts-api-utils "^1.0.1" | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "8.12.2" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/typescript-estree/-/typescript-estree-8.12.2.tgz#206df9b1cbff212aaa9401985ef99f04daa84da5" | ||
| integrity sha512-mME5MDwGe30Pq9zKPvyduyU86PH7aixwqYR2grTglAdB+AN8xXQ1vFGpYaUSJ5o5P/5znsSBeNcs5g5/2aQwow== | ||
| dependencies: | ||
| "@typescript-eslint/types" "8.12.2" | ||
| "@typescript-eslint/visitor-keys" "8.12.2" | ||
| debug "^4.3.4" | ||
| fast-glob "^3.3.2" | ||
| is-glob "^4.0.3" | ||
| minimatch "^9.0.4" | ||
| semver "^7.6.0" | ||
| ts-api-utils "^1.3.0" | ||
|
|
||
| "@typescript-eslint/utils@^8.12.2": | ||
| version "8.12.2" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/utils/-/utils-8.12.2.tgz#726cc9f49f5866605bd15bbc1768ffc15637930e" | ||
| integrity sha512-UTTuDIX3fkfAz6iSVa5rTuSfWIYZ6ATtEocQ/umkRSyC9O919lbZ8dcH7mysshrCdrAM03skJOEYaBugxN+M6A== | ||
| dependencies: | ||
| "@eslint-community/eslint-utils" "^4.4.0" | ||
| "@typescript-eslint/scope-manager" "8.12.2" | ||
| "@typescript-eslint/types" "8.12.2" | ||
| "@typescript-eslint/typescript-estree" "8.12.2" | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "7.3.1" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/visitor-keys/-/visitor-keys-7.3.1.tgz#6ddef14a3ce2a79690f01176f5305c34d7b93d8c" | ||
|
|
@@ -1196,6 +1240,14 @@ | |
| "@typescript-eslint/types" "7.3.1" | ||
| eslint-visitor-keys "^3.4.1" | ||
|
|
||
| "@typescript-eslint/[email protected]": | ||
| version "8.12.2" | ||
| resolved "https://registry.yarnpkg.com/@typescript-eslint/visitor-keys/-/visitor-keys-8.12.2.tgz#94d7410f78eb6d134b9fcabaf1eeedb910ba8c38" | ||
| integrity sha512-PChz8UaKQAVNHghsHcPyx1OMHoFRUEA7rJSK/mDhdq85bk+PLsUHUBqTQTFt18VJZbmxBovM65fezlheQRsSDA== | ||
| dependencies: | ||
| "@typescript-eslint/types" "8.12.2" | ||
| eslint-visitor-keys "^3.4.3" | ||
|
|
||
| "@zkochan/cmd-shim@^3.1.0": | ||
| version "3.1.0" | ||
| resolved "https://registry.yarnpkg.com/@zkochan/cmd-shim/-/cmd-shim-3.1.0.tgz#2ab8ed81f5bb5452a85f25758eb9b8681982fd2e" | ||
|
|
@@ -2604,7 +2656,7 @@ eslint-visitor-keys@^3.3.0, eslint-visitor-keys@^3.4.0: | |
| resolved "https://registry.yarnpkg.com/eslint-visitor-keys/-/eslint-visitor-keys-3.4.0.tgz#c7f0f956124ce677047ddbc192a68f999454dedc" | ||
| integrity sha512-HPpKPUBQcAsZOsHAFwTtIKcYlCje62XB7SEAcxjtmW6TD1WVpkS6i6/hOVtTZIl4zGj/mBqpFVGvaDneik+VoQ== | ||
|
|
||
| eslint-visitor-keys@^3.4.1, eslint-visitor-keys@^3.4.3: | ||
| version "3.4.3" | ||
| resolved "https://registry.yarnpkg.com/eslint-visitor-keys/-/eslint-visitor-keys-3.4.3.tgz#0cd72fe8550e3c2eae156a96a4dddcd1c8ac5800" | ||
| integrity sha512-wpc+LXeiyiisxPlEkUzU6svyS1frIO3Mgxj1fdy7Pm8Ygzguax2N3Fa/D/ag1WqbOprdI+uY6wMUl8/a2G+iag== | ||
|
|
@@ -2822,6 +2874,17 @@ fast-glob@^3.2.11, fast-glob@^3.2.9: | |
| merge2 "^1.3.0" | ||
| micromatch "^4.0.4" | ||
|
|
||
| fast-glob@^3.3.2: | ||
| version "3.3.2" | ||
| resolved "https://registry.yarnpkg.com/fast-glob/-/fast-glob-3.3.2.tgz#a904501e57cfdd2ffcded45e99a54fef55e46129" | ||
| integrity sha512-oX2ruAFQwf/Orj8m737Y5adxDQO0LAB7/S5MnxCdTNDd4p6BsyIVsv9JQsATbTSq8KHRpLwIHbVlUNatxd+1Ow== | ||
| dependencies: | ||
| "@nodelib/fs.stat" "^2.0.2" | ||
| "@nodelib/fs.walk" "^1.2.3" | ||
| glob-parent "^5.1.2" | ||
| merge2 "^1.3.0" | ||
| micromatch "^4.0.4" | ||
|
|
||
| fast-json-stable-stringify@^2.0.0: | ||
| version "2.1.0" | ||
| resolved "https://registry.yarnpkg.com/fast-json-stable-stringify/-/fast-json-stable-stringify-2.1.0.tgz#874bf69c6f404c2b5d99c481341399fd55892633" | ||
|
|
@@ -4583,6 +4646,13 @@ minimatch@^9.0.1: | |
| dependencies: | ||
| brace-expansion "^2.0.1" | ||
|
|
||
| minimatch@^9.0.4: | ||
| version "9.0.5" | ||
| resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-9.0.5.tgz#d74f9dd6b57d83d8e98cfb82133b03978bc929e5" | ||
| integrity sha512-G6T0ZX48xgozx7587koeX9Ys2NYy6Gmv//P89sEte9V9whIapMNF4idKxnW2QtCcLiTWlb/wfCabAtAFWhhBow== | ||
| dependencies: | ||
| brace-expansion "^2.0.1" | ||
|
|
||
| minimist-options@^3.0.1: | ||
| version "3.0.2" | ||
| resolved "https://registry.yarnpkg.com/minimist-options/-/minimist-options-3.0.2.tgz#fba4c8191339e13ecf4d61beb03f070103f3d954" | ||
|
|
@@ -6012,6 +6082,11 @@ semver@^7.5.4: | |
| dependencies: | ||
| lru-cache "^6.0.0" | ||
|
|
||
| semver@^7.6.0: | ||
| version "7.6.3" | ||
| resolved "https://registry.yarnpkg.com/semver/-/semver-7.6.3.tgz#980f7b5550bc175fb4dc09403085627f9eb33143" | ||
| integrity sha512-oVekP1cKtI+CTDvHWYFUcMtsK/00wmAEfyqKfNdARm8u1wNVhSgaX7A8d4UuIlUI5e84iEwOhs7ZPYRmzU9U6A== | ||
|
|
||
| sentence-case@^3.0.4: | ||
| version "3.0.4" | ||
| resolved "https://registry.yarnpkg.com/sentence-case/-/sentence-case-3.0.4.tgz#3645a7b8c117c787fde8702056225bb62a45131f" | ||
|
|
@@ -6682,7 +6757,7 @@ trim-off-newlines@^1.0.0: | |
| resolved "https://registry.yarnpkg.com/trim-off-newlines/-/trim-off-newlines-1.0.3.tgz#8df24847fcb821b0ab27d58ab6efec9f2fe961a1" | ||
| integrity sha512-kh6Tu6GbeSNMGfrrZh6Bb/4ZEHV1QlB4xNDBeog8Y9/QwFlKTRyWvY3Fs9tRDAMZliVUwieMgEdIeL/FtqjkJg== | ||
|
|
||
| ts-api-utils@^1.0.1, ts-api-utils@^1.3.0: | ||
| version "1.3.0" | ||
| resolved "https://registry.yarnpkg.com/ts-api-utils/-/ts-api-utils-1.3.0.tgz#4b490e27129f1e8e686b45cc4ab63714dc60eea1" | ||
| integrity sha512-UQMIo7pb8WRomKR1/+MFVLTroIvDVtMX3K6OUir8ynLyzB8Jeriont2bTAtmNPa1ekAgN7YPDyf6V+ygrdU+eQ== | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but wondering if we should add a directory to
helpersfor these sort of default message functions.defaultMessagesas the directory ->renameIdentifier.tsxas this helper for example. Or just yanking this out as its own individual helper to be reused elsewhere.