-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(FormFiledGroupHeaderTitleTextObject): interface renamed (typo) #610
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
18 changes: 18 additions & 0 deletions
18
...roupHeaderTitleTextObjectRenamed/formFiledGroupHeaderTitleTextObject-renamed.md
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 @@ | ||
### formFiledGroupHeaderTitleTextObject-renamed [(#10016)](https://github.com/patternfly/patternfly-react/pull/10016) | ||
|
||
There was a typo in FormFiledGroupHeaderTitleTextObject interface. It was renamed to the intended FormFieldGroupHeaderTitleTextObject. | ||
|
||
#### Examples | ||
|
||
In: | ||
|
||
```jsx | ||
%inputExample% | ||
``` | ||
|
||
Out: | ||
|
||
```jsx | ||
%outputExample% | ||
``` | ||
|
43 changes: 43 additions & 0 deletions
43
...iledGroupHeaderTitleTextObjectRenamed/formFiledGroupHeaderTitleTextObject-renamed.test.ts
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,43 @@ | ||
const ruleTester = require("../../ruletester"); | ||
import * as rule from "./formFiledGroupHeaderTitleTextObject-renamed"; | ||
|
||
const errorMessage = | ||
"There was a typo in FormFiledGroupHeaderTitleTextObject interface. It was renamed to the intended FormFieldGroupHeaderTitleTextObject."; | ||
|
||
ruleTester.run("formFiledGroupHeaderTitleTextObject-renamed", rule, { | ||
valid: [ | ||
{ | ||
code: `const titleTextObject: FormFiledGroupHeaderTitleTextObject = {text: "Some title", id: "form-field-group-header-title-text"};`, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: `import { FormFiledGroupHeaderTitleTextObject } from '@patternfly/react-core'; const titleTextObject: FormFiledGroupHeaderTitleTextObject = {text: "Some title", id: "form-field-group-header-title-text"};`, | ||
output: `import { FormFieldGroupHeaderTitleTextObject } from '@patternfly/react-core'; const titleTextObject: FormFieldGroupHeaderTitleTextObject = {text: "Some title", id: "form-field-group-header-title-text"};`, | ||
errors: [ | ||
{ | ||
message: errorMessage, | ||
type: "ImportSpecifier", | ||
}, | ||
{ | ||
message: errorMessage, | ||
type: "TSTypeReference", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { FormFiledGroupHeaderTitleTextObject } from '@patternfly/react-core'; interface MyExtension extends FormFiledGroupHeaderTitleTextObject {}`, | ||
output: `import { FormFieldGroupHeaderTitleTextObject } from '@patternfly/react-core'; interface MyExtension extends FormFieldGroupHeaderTitleTextObject {}`, | ||
errors: [ | ||
{ | ||
message: errorMessage, | ||
type: "ImportSpecifier", | ||
}, | ||
{ | ||
message: errorMessage, | ||
type: "TSInterfaceHeritage", | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
59 changes: 59 additions & 0 deletions
59
...formFiledGroupHeaderTitleTextObjectRenamed/formFiledGroupHeaderTitleTextObject-renamed.ts
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,59 @@ | ||
import { AST, Rule } from "eslint"; | ||
import { ImportSpecifier, Identifier, Node } from "estree-jsx"; | ||
import { getFromPackage } from "../../helpers"; | ||
|
||
// https://github.com/patternfly/patternfly-react/pull/10016 | ||
module.exports = { | ||
meta: { fixable: "code" }, | ||
create: function (context: Rule.RuleContext) { | ||
const { imports } = getFromPackage(context, "@patternfly/react-core"); | ||
|
||
const previousName = "FormFiledGroupHeaderTitleTextObject"; | ||
const newName = "FormFieldGroupHeaderTitleTextObject"; | ||
|
||
const interfaceImport = imports.find( | ||
(specifier) => specifier.imported.name === previousName | ||
); | ||
|
||
if (!interfaceImport) { | ||
return {}; | ||
} | ||
|
||
const hasAlias = (specifier: ImportSpecifier) => | ||
specifier.local.name !== specifier.imported.name; | ||
|
||
const reportMessage = | ||
"There was a typo in FormFiledGroupHeaderTitleTextObject interface. It was renamed to the intended FormFieldGroupHeaderTitleTextObject."; | ||
|
||
const callContextReport = (node: Node, nodeToReplace: Node | AST.Token) => { | ||
context.report({ | ||
node, | ||
message: reportMessage, | ||
fix(fixer) { | ||
return fixer.replaceText(nodeToReplace, newName); | ||
}, | ||
}); | ||
}; | ||
|
||
return { | ||
ImportSpecifier(node: ImportSpecifier) { | ||
if (node.imported.name === interfaceImport.imported.name) { | ||
callContextReport(node, node.imported); | ||
} | ||
}, | ||
TSTypeReference(node: { typeName: Identifier }) { | ||
if (!hasAlias(interfaceImport) && node.typeName.name === previousName) { | ||
callContextReport(node as unknown as Node, node.typeName); | ||
} | ||
}, | ||
TSInterfaceHeritage(node: { expression: Identifier }) { | ||
if ( | ||
!hasAlias(interfaceImport) && | ||
node.expression.name === previousName | ||
) { | ||
callContextReport(node as unknown as Node, node.expression); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
8 changes: 8 additions & 0 deletions
8
...iledGroupHeaderTitleTextObjectRenamed/formFiledGroupHeaderTitleTextObjectRenamedInput.tsx
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,8 @@ | ||
import { FormFiledGroupHeaderTitleTextObject } from "@patternfly/react-core"; | ||
|
||
interface MyExtension extends FormFiledGroupHeaderTitleTextObject {} | ||
|
||
const titleTextObject: FormFiledGroupHeaderTitleTextObject = { | ||
text: "Some title", | ||
id: "form-field-group-header-title-text", | ||
}; |
8 changes: 8 additions & 0 deletions
8
...ledGroupHeaderTitleTextObjectRenamed/formFiledGroupHeaderTitleTextObjectRenamedOutput.tsx
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,8 @@ | ||
import { FormFieldGroupHeaderTitleTextObject } from "@patternfly/react-core"; | ||
|
||
interface MyExtension extends FormFieldGroupHeaderTitleTextObject {} | ||
|
||
const titleTextObject: FormFieldGroupHeaderTitleTextObject = { | ||
text: "Some title", | ||
id: "form-field-group-header-title-text", | ||
}; |
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.
Having these typed as objects with just the expected values and then asserting that they're Nodes feels odd, are there not better types we could use here?
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.
Yeah, I know. I was struggling to find any type for these, there are only enums in
/packages/eslint-plugin-pf-codemods/lib/rules/ast-node-types.d.ts
which I think is copied from@typescript-eslint/parser
. But I am taking a look at the typescript-eslint repo now and it seems the actual interface might be exported there.I'll try to import that
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.
So, the type we want is in this file: typescript-eslint repo, but it is a part of an internal package ast-spec.
After installing
typescript-eslint
, it does not export any of the AST node types from the node_modules.Trying to install the internal ast-spec package directly with
yarn add @typescript-eslint/ast-spec --dev
does not work.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.
Hm 🤔
Let me do a little hunting myself and see if I can find the types we need exported anywhere.
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.
I think I found them:
import { TSTypeReference, TSInterfaceHeritage} from '@typescript-eslint/parser'
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.
I tried importing this, the '@typescript-eslint/parser' package should be installed, but it says it is not exported.
Updated
packages/eslint-plugin-pf-codemods/package.json
:Then I did
yarn install
I don't know if I am installing something wrong, but it just seems like it is not exported.
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.
I'm seeing the same thing but I swear this was working on Friday 🤔
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.
I really am sure that this was working Friday and now I'm so confused 😆
It looks like types of those names can be retrieved from
import { TSESTree } from '@typescript-eslint/types'
... but they don't slide right into place and I'm not sure if those are the wrong type or if the current logic isn't fully type safe.I do feel like we should investigate this further but I won't block this PR over it if you'd like to make a followup issue so that we don't forget about it.
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.
Typescript contributors are playing some high-level games with us I guess 😃
Anyways, the
TSESTree.AST_NODE_TYPES
enum si fine, but it does not fit the use case as you say. I'd also not block over, the codemod should work even if typed asTSTypeReference(node: any) {
, as if the node matches that TSTypeReference function, it should already have the suitable structure I suppose.Opened the issue for further investigation: #622
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.
Yeah the codemod will still work, which is why I'm fine with the typing being patched in a followup. It just isn't ideal from a typing perspective since it isn't really valid, we're just telling TS to trust us and chill out 😆