-
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(Masthead): Update subcomponent names #729
Changes from 1 commit
36e7225
b3d1e39
c1d2b71
1928c19
b448397
b600a00
fb3ca19
9fc4352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { JSXAttribute } from "estree-jsx"; | ||
|
||
/** Gets the name value of a JSX attribute */ | ||
export function getAttributeName(attr: JSXAttribute) { | ||
switch (attr.name.type) { | ||
case "JSXIdentifier": | ||
return attr.name.name; | ||
case "JSXNamespacedName": | ||
return attr.name.name.name; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import { ImportSpecifier } from "estree-jsx"; | ||
import { ImportDefaultSpecifierWithParent } from "./interfaces"; | ||
import { getDeclarationString } from "./getDeclarationString"; | ||
|
||
/** Gets the name of an import based on the specifier and an array of names that should be looked for in default import paths */ | ||
export function getComponentImportName( | ||
importSpecifier: ImportSpecifier | ImportDefaultSpecifierWithParent, | ||
potentialNames: string[] | ||
) { | ||
if (importSpecifier.type === "ImportSpecifier") { | ||
return importSpecifier.imported.name; | ||
} | ||
|
||
return potentialNames.find((name) => | ||
getDeclarationString(importSpecifier)?.includes(name) | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { ImportDefaultSpecifierWithParent } from "./interfaces"; | ||
|
||
/** Gets the import path string for a default import */ | ||
export function getDeclarationString( | ||
defaultImportSpecifier: ImportDefaultSpecifierWithParent | ||
) { | ||
return defaultImportSpecifier?.parent?.source.value?.toString(); | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,23 @@ | ||||||
import { JSXOpeningElement, JSXMemberExpression } from "estree-jsx"; | ||||||
|
||||||
/** Gets the name of an opening element or member expression */ | ||||||
export function getNodeName(node: JSXOpeningElement | JSXMemberExpression) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'd rather separate the JSXMemberExpression handling into a separate function. Because if I am not mistaken, getNodeName will be called from the outside only with a node of type We can have it as a private function in this file I think.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So my idea with just naming this helper I'm not against splitting the logic into discrete private functions and minimizing recursion to help improve readability at all though 🙂 CC @thatblindgeye since you left a comment about this same kind of stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going off of Adam's comment above, another nit so not a blocker but if this helper is meant solely for these 2 scenarios, might be worth updating the name to reflect that (if we can without it become tooooo verbose). |
||||||
if (node.type === "JSXMemberExpression") { | ||||||
switch (node.object.type) { | ||||||
case "JSXMemberExpression": | ||||||
return getNodeName(node.object); | ||||||
case "JSXIdentifier": | ||||||
return node.object.name; | ||||||
} | ||||||
} | ||||||
|
||||||
switch (node.name.type) { | ||||||
case "JSXMemberExpression": | ||||||
return getNodeName(node.name); | ||||||
case "JSXIdentifier": | ||||||
case "JSXNamespacedName": | ||||||
return typeof node.name.name === "string" | ||||||
? node.name.name | ||||||
: node.name.name.name; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { JSXAttribute, JSXOpeningElement } from "estree-jsx"; | ||
import { getAttributeName } from "./getAttributeName"; | ||
|
||
/** Returns true if the passed opening element has a data-codemods attribute */ | ||
export function hasCodeModDataTag(openingElement: JSXOpeningElement) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could always be a separate helper, but just wondering if we should only check if the data tag exists by name, or return the actual attribute and base conditional logic on that. Just thinking if we ever start supplying specific values to a |
||
const nonSpreadAttributes = openingElement.attributes.filter( | ||
(attr) => attr.type === "JSXAttribute" | ||
); | ||
const attributeNames = nonSpreadAttributes.map((attr) => | ||
getAttributeName(attr as JSXAttribute) | ||
); | ||
return attributeNames.includes("data-codemods"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
import { Rule } from "eslint"; | ||
import { ImportSpecifier } from "estree-jsx"; | ||
import { getAllImportsFromPackage } from "./getFromPackage"; | ||
import { | ||
ImportDefaultSpecifierWithParent, | ||
JSXOpeningElementWithParent, | ||
} from "./interfaces"; | ||
import { | ||
getDeclarationString, | ||
getComponentImportName, | ||
getNodeName, | ||
hasCodeModDataTag, | ||
} from "./index"; | ||
|
||
interface ComponentRenames { | ||
[currentName: string]: string; | ||
} | ||
|
||
function formatDefaultMessage(oldName: string, newName: string) { | ||
return `${oldName} has been renamed to ${newName}.`; | ||
} | ||
|
||
function getFixes( | ||
fixer: Rule.RuleFixer, | ||
nodeImport: ImportSpecifier | ImportDefaultSpecifierWithParent, | ||
node: JSXOpeningElementWithParent, | ||
oldName: string, | ||
newName: string | ||
) { | ||
const fixes = []; | ||
|
||
const isNamedImport = nodeImport.type === "ImportSpecifier"; | ||
if (isNamedImport) { | ||
fixes.push(fixer.replaceText(nodeImport.imported, newName)); | ||
} else { | ||
const importDeclaration = nodeImport.parent; | ||
const newImportDeclaration = importDeclaration?.source.raw?.replace( | ||
oldName, | ||
newName | ||
); | ||
if (importDeclaration && newImportDeclaration) { | ||
fixes.push( | ||
fixer.replaceText(importDeclaration.source, newImportDeclaration) | ||
); | ||
} | ||
} | ||
|
||
const shouldRenameNode = | ||
isNamedImport && nodeImport.imported.name === nodeImport.local.name; | ||
|
||
if (shouldRenameNode) { | ||
fixes.push(fixer.replaceText(node.name, newName)); | ||
fixes.push(fixer.insertTextAfter(node.name, " data-codemods")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this should be optional, I know we need the But if anything, it can be done as a followup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I went back and forth on that and IIRC talked it over with @thatblindgeye, I think we landed on always adding it to prevent issues and adding #723 so that we aren't leaving consumer codebases littered with our data tags. I'm not super opposed to making it optional though, but yeah would want to do it as a followup. |
||
} | ||
|
||
const closingElement = node?.parent?.closingElement; | ||
if (shouldRenameNode && closingElement) { | ||
fixes.push(fixer.replaceText(closingElement.name, newName)); | ||
} | ||
|
||
return fixes; | ||
} | ||
|
||
export function renameComponent( | ||
renames: ComponentRenames, | ||
packageName = "@patternfly/react-core" | ||
) { | ||
return function (context: Rule.RuleContext) { | ||
const oldNames = Object.keys(renames); | ||
const imports = getAllImportsFromPackage(context, packageName, oldNames); | ||
|
||
if (imports.length === 0) { | ||
return {}; | ||
} | ||
|
||
return { | ||
JSXOpeningElement(node: JSXOpeningElementWithParent) { | ||
if (hasCodeModDataTag(node)) { | ||
return; | ||
} | ||
|
||
const nodeName = getNodeName(node); | ||
const nodeImport = imports.find((imp) => { | ||
if (imp.type === "ImportSpecifier") { | ||
return [imp.imported.name, imp.local.name].includes(nodeName); | ||
} | ||
|
||
return oldNames.some((name) => | ||
getDeclarationString(imp)?.includes(name) | ||
); | ||
}); | ||
|
||
if (!nodeImport) { | ||
return; | ||
} | ||
|
||
const oldName = getComponentImportName(nodeImport, oldNames); | ||
|
||
if (!oldName) { | ||
return; | ||
} | ||
|
||
const newName = renames[oldName]; | ||
|
||
context.report({ | ||
node, | ||
message: formatDefaultMessage(oldName, newName), | ||
fix: (fixer) => getFixes(fixer, nodeImport, node, oldName, newName), | ||
}); | ||
}, | ||
}; | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
### masthead-name-changes [(#10809)](https://github.com/patternfly/patternfly-react/pull/10809) | ||
|
||
The old `MastheadBrand` component has been renamed to `MastheadLogo`, and the old `MastheadMain` has been renamed to `MastheadBrand`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the message might be confusing for some consumers (or at least the messages given in the codemod output "MastheadMain has been renamed to MastheadBrand"), I like using the word "old" here, which makes more sense. I was confused at the first place why are we renaming these components, when they will still stay in the new Masthead. Maybe we can add a sentence here, like " I know there will probably be a second codemod for wrapping the components in MastheadMain and the message there will better describe the whole change, but still this renaming which is not really renaming might be confusing (but probably not many consumers read the messages, I guess they just run the fix, so I am ok with keeping it as is). |
||
|
||
#### Examples | ||
|
||
In: | ||
|
||
```jsx | ||
%inputExample% | ||
``` | ||
|
||
Out: | ||
|
||
```jsx | ||
%outputExample% | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
const ruleTester = require("../../ruletester"); | ||
import * as rule from "./masthead-name-changes"; | ||
|
||
ruleTester.run("masthead-name-changes", rule, { | ||
valid: [ | ||
{ | ||
code: `<MastheadBrand />`, | ||
}, | ||
{ | ||
code: `<MastheadMain />`, | ||
}, | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core'; <MastheadBrand data-codemods />`, | ||
}, | ||
{ | ||
code: `import { MastheadMain } from '@patternfly/react-core'; <MastheadMain data-codemods />`, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core'; <MastheadBrand />`, | ||
output: `import { MastheadLogo } from '@patternfly/react-core'; <MastheadLogo data-codemods />`, | ||
errors: [ | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { MastheadMain } from '@patternfly/react-core'; <MastheadMain />`, | ||
output: `import { MastheadBrand } from '@patternfly/react-core'; <MastheadBrand data-codemods />`, | ||
errors: [ | ||
{ | ||
message: `MastheadMain has been renamed to MastheadBrand.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
// with other props | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core'; <MastheadBrand className="foo" />`, | ||
output: `import { MastheadLogo } from '@patternfly/react-core'; <MastheadLogo data-codemods className="foo" />`, | ||
errors: [ | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
// because of how the unit tests run I have to handle having both MastheadBrand and MastheadMain together in stages | ||
{ | ||
code: `import { MastheadBrand, MastheadMain } from '@patternfly/react-core'; <MastheadMain><MastheadBrand>Logo</MastheadBrand></MastheadMain>`, | ||
output: `import { MastheadLogo, MastheadMain } from '@patternfly/react-core'; <MastheadMain><MastheadLogo data-codemods>Logo</MastheadLogo></MastheadMain>`, | ||
errors: [ | ||
{ | ||
message: `MastheadMain has been renamed to MastheadBrand.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { MastheadLogo, MastheadMain } from '@patternfly/react-core'; <MastheadMain><MastheadLogo data-codemods>Logo</MastheadLogo></MastheadMain>`, | ||
output: `import { MastheadLogo, MastheadBrand } from '@patternfly/react-core'; <MastheadBrand data-codemods><MastheadLogo data-codemods>Logo</MastheadLogo></MastheadBrand>`, | ||
errors: [ | ||
{ | ||
message: `MastheadMain has been renamed to MastheadBrand.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
// with alias | ||
{ | ||
code: `import { MastheadBrand as MB } from '@patternfly/react-core'; <MB />`, | ||
output: `import { MastheadLogo as MB } from '@patternfly/react-core'; <MB />`, | ||
errors: [ | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
// dist imports | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core/dist/esm/components/Masthead/MastheadBrand'; <MastheadBrand />`, | ||
output: `import { MastheadLogo } from '@patternfly/react-core/dist/esm/components/Masthead/MastheadBrand'; <MastheadLogo data-codemods />`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the import path be changed to |
||
errors: [ | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core/dist/js/components/Masthead/MastheadBrand'; <MastheadBrand />`, | ||
output: `import { MastheadLogo } from '@patternfly/react-core/dist/js/components/Masthead/MastheadBrand'; <MastheadLogo data-codemods />`, | ||
errors: [ | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core/dist/dynamic/components/Masthead/MastheadBrand'; <MastheadBrand />`, | ||
output: `import { MastheadLogo } from '@patternfly/react-core/dist/dynamic/components/Masthead/MastheadBrand'; <MastheadLogo data-codemods />`, | ||
errors: [ | ||
{ | ||
message: `MastheadBrand has been renamed to MastheadLogo.`, | ||
type: "JSXOpeningElement", | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { renameComponent } from "../../helpers/renameComponent"; | ||
|
||
// https://github.com/patternfly/patternfly-react/pull/10809 | ||
|
||
const renames = { | ||
MastheadBrand: "MastheadLogo", | ||
MastheadMain: "MastheadBrand", | ||
}; | ||
|
||
module.exports = { | ||
meta: { fixable: "code" }, | ||
create: renameComponent(renames), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { MastheadBrand, MastheadMain } from "@patternfly/react-core"; | ||
|
||
export const MastheadNameChanges = () => ( | ||
<MastheadMain> | ||
<MastheadBrand>Logo</MastheadBrand> | ||
</MastheadMain> | ||
); |
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.
More of a nit so not a blocker, but would
getDefaultDeclarationString
or something similar be more accurate?