Skip to content
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 1 commit into from
Mar 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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%
```

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",
},
],
},
],
});
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 }) {
Comment on lines +44 to +49
Copy link
Collaborator

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?

Copy link
Contributor Author

@adamviktora adamviktora Mar 21, 2024

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

Copy link
Contributor Author

@adamviktora adamviktora Mar 21, 2024

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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'

Copy link
Contributor Author

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.

Screenshot 2024-03-25 at 11 27 55

Updated packages/eslint-plugin-pf-codemods/package.json:

Screenshot 2024-03-25 at 11 27 04

Then I did yarn install
I don't know if I am installing something wrong, but it just seems like it is not exported.

Copy link
Collaborator

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 🤔

Copy link
Collaborator

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.

Copy link
Contributor Author

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 as TSTypeReference(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

Copy link
Collaborator

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 😆

if (
!hasAlias(interfaceImport) &&
node.expression.name === previousName
) {
callContextReport(node as unknown as Node, node.expression);
}
},
};
},
};
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",
};
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",
};
Loading