-
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 all commits
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,13 @@ | ||
import { JSXAttribute, JSXOpeningElement } from "estree-jsx"; | ||
import { getAttributeName } from "./getAttributeName"; | ||
|
||
/** Returns the data-codemods attribute of an element, if it exists */ | ||
export function getCodeModDataTag(openingElement: JSXOpeningElement) { | ||
const nonSpreadAttributes = openingElement.attributes.filter( | ||
(attr) => attr.type === "JSXAttribute" | ||
); | ||
|
||
return nonSpreadAttributes.find( | ||
(attr) => getAttributeName(attr as JSXAttribute) === "data-codemods" | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import { ImportSpecifier } from "estree-jsx"; | ||
import { ImportDefaultSpecifierWithParent } from "./interfaces"; | ||
import { getDefaultDeclarationString } from "./getDefaultDeclarationString"; | ||
|
||
/** 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) => | ||
getDefaultDeclarationString(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 getDefaultDeclarationString( | ||
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"; | ||
|
||
function getJSXMemberExpressionName(node: JSXMemberExpression) { | ||
switch (node.object.type) { | ||
case "JSXMemberExpression": | ||
return getJSXMemberExpressionName(node.object); | ||
case "JSXIdentifier": | ||
return node.object.name; | ||
} | ||
} | ||
|
||
/** Gets the name of an opening element */ | ||
export function getNodeName(node: JSXOpeningElement) { | ||
switch (node.name.type) { | ||
case "JSXMemberExpression": | ||
return getJSXMemberExpressionName(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,7 @@ | ||
import { JSXOpeningElement } from "estree-jsx"; | ||
import { getCodeModDataTag } from "./getCodeModDataTag"; | ||
|
||
/** Returns true if the passed opening element has a data-codemods attribute */ | ||
export function hasCodeModDataTag(openingElement: JSXOpeningElement) { | ||
return !!getCodeModDataTag(openingElement) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,25 @@ | ||
import { | ||
Node, | ||
Identifier, | ||
ImportSpecifier, | ||
ImportDefaultSpecifier, | ||
ImportDeclaration, | ||
JSXOpeningElement, | ||
JSXElement, | ||
} from "estree-jsx"; | ||
|
||
export interface IdentifierWithParent extends Identifier { | ||
parent?: Node; | ||
} | ||
|
||
export interface ImportSpecifierWithParent extends ImportSpecifier { | ||
parent?: ImportDeclaration; | ||
} | ||
export interface ImportDefaultSpecifierWithParent | ||
extends ImportDefaultSpecifier { | ||
parent?: ImportDeclaration; | ||
} | ||
|
||
export interface JSXOpeningElementWithParent extends JSXOpeningElement { | ||
parent?: JSXElement; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
import { Rule } from "eslint"; | ||
import { getAllImportsFromPackage } from "./getFromPackage"; | ||
import { | ||
ImportSpecifierWithParent, | ||
ImportDefaultSpecifierWithParent, | ||
JSXOpeningElementWithParent, | ||
} from "./interfaces"; | ||
import { | ||
getDefaultDeclarationString, | ||
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: ImportSpecifierWithParent | ImportDefaultSpecifierWithParent, | ||
node: JSXOpeningElementWithParent, | ||
oldName: string, | ||
newName: string | ||
) { | ||
const fixes = []; | ||
|
||
const isNamedImport = nodeImport.type === "ImportSpecifier"; | ||
const importDeclaration = nodeImport.parent; | ||
const importSource = importDeclaration?.source.raw; | ||
const importSourceHasComponentName = importSource?.includes(oldName); | ||
const newImportDeclaration = importSource?.replace(oldName, newName); | ||
|
||
if (isNamedImport) { | ||
fixes.push(fixer.replaceText(nodeImport.imported, newName)); | ||
} | ||
|
||
if ( | ||
importDeclaration && | ||
newImportDeclaration && | ||
importSourceHasComponentName | ||
) { | ||
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) => | ||
getDefaultDeclarationString(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,20 @@ | ||
### masthead-name-changes [(#10809)](https://github.com/patternfly/patternfly-react/pull/10809) | ||
|
||
Our old implementation of `MastheadBrand` has been renamed to `MastheadLogo`, which must be wrapped by our new implementation of `MastheadBrand`." | ||
|
||
This rule will handle the renaming, required restructuring will be handled under a separate rule. | ||
|
||
#### Examples | ||
|
||
In: | ||
|
||
```jsx | ||
%inputExample% | ||
``` | ||
|
||
Out: | ||
|
||
```jsx | ||
%outputExample% | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
const ruleTester = require("../../ruletester"); | ||
import * as rule from "./masthead-name-changes"; | ||
|
||
ruleTester.run("masthead-name-changes", rule, { | ||
valid: [ | ||
{ | ||
code: `<MastheadBrand />`, | ||
}, | ||
{ | ||
code: `import { MastheadBrand } from '@patternfly/react-core'; <MastheadBrand 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", | ||
}, | ||
], | ||
}, | ||
// 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", | ||
}, | ||
], | ||
}, | ||
// 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/MastheadLogo'; <MastheadLogo data-codemods />`, | ||
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/MastheadLogo'; <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/MastheadLogo'; <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,12 @@ | ||
import { renameComponent } from "../../helpers/renameComponent"; | ||
|
||
// https://github.com/patternfly/patternfly-react/pull/10809 | ||
|
||
const renames = { | ||
MastheadBrand: "MastheadLogo", | ||
}; | ||
|
||
module.exports = { | ||
meta: { fixable: "code" }, | ||
create: renameComponent(renames), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { MastheadBrand } from "@patternfly/react-core"; | ||
|
||
export const MastheadNameChanges = () => <MastheadBrand>Logo</MastheadBrand>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { MastheadLogo } from "@patternfly/react-core"; | ||
|
||
export const MastheadNameChanges = () => <MastheadLogo data-codemods>Logo</MastheadLogo>; |
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.
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
data-codemods
attribute, e.g. maybe we only want to skip/run a rule if an openingElement hasdata-codemods="renamed"
, but notdata-codemods="deprecated"
or something.