fix(buttonMoveIconsIconProp): make elements self-closing (#728)

- also fixes Icon imported with alias
- supports Icon / react-icons being used in children attribute

packages/eslint-plugin-pf-codemods/src/rules/helpers/JSXAttributes.ts

@@ -1,7 +1,10 @@
 import { Rule, Scope } from "eslint";
 import {
-  JSXElement,
+  Expression,
   JSXAttribute,
+  JSXElement,
+  JSXEmptyExpression,
+  JSXFragment,
   JSXOpeningElement,
   MemberExpression,
 } from "estree-jsx";
@@ -68,7 +71,7 @@ export function getExpression(node?: JSXAttribute["value"]) {
   }
 
   if (node.type === "JSXExpressionContainer") {
-    return node.expression;
+    return node.expression as Expression | JSXEmptyExpression | JSXFragment;
   }
 }
 

packages/eslint-plugin-pf-codemods/src/rules/helpers/childrenIsEmpty.ts

@@ -0,0 +1,11 @@
+import { JSXElement } from "estree-jsx";
+
+/** Checks whether children is empty (no children or only whitespaces) */
+export function childrenIsEmpty(children: JSXElement["children"]) {
+  return (
+    !children.length ||
+    (children.length === 1 &&
+      children[0].type === "JSXText" &&
+      children[0].value.trim() === "")
+  );
+}

packages/eslint-plugin-pf-codemods/src/rules/helpers/getChildJSXElementByName.ts

@@ -25,7 +25,10 @@ function getChildJSXElementCallback(
 /** Can be used to run logic if the specific child element exists, or to run logic on the
  * specified element.
  */
-export function getChildJSXElementByName(node: JSXElement, name: string) {
+export function getChildJSXElementByName(
+  node: JSXElement | JSXFragment,
+  name: string
+) {
   return node.children?.find((child) =>
     getChildJSXElementCallback(child, name)
   ) as JSXElement | undefined;
@@ -34,7 +37,10 @@ export function getChildJSXElementByName(node: JSXElement, name: string) {
 /** Can be used to run logic if the specific child elements exist, or to run logic on the
  * specified elements.
  */
-export function getAllChildJSXElementsByName(node: JSXElement, name: string) {
+export function getAllChildJSXElementsByName(
+  node: JSXElement | JSXFragment,
+  name: string
+) {
   return node.children?.filter((child) =>
     getChildJSXElementCallback(child, name)
   ) as JSXElement[];

packages/eslint-plugin-pf-codemods/src/rules/helpers/index.ts

@@ -1,3 +1,4 @@
+export * from "./childrenIsEmpty";
 export * from "./contextReports";
 export * from "./findAncestor";
 export * from "./fixers";
@@ -22,6 +23,7 @@ export * from "./includesImport";
 export * from "./interfaces";
 export * from "./isReactIcon";
 export * from "./JSXAttributes";
+export * from "./makeJSXElementSelfClosing";
 export * from "./nodeMatches";
 export * from "./pfPackageMatches";
 export * from "./removeElement";

packages/eslint-plugin-pf-codemods/src/rules/helpers/makeJSXElementSelfClosing.ts

@@ -0,0 +1,47 @@
+import { Rule } from "eslint";
+import { JSXElement, JSXText } from "estree-jsx";
+import { childrenIsEmpty } from ".";
+
+/** Transforms JSXElement to a self-closing tag.
+ * Works only on elements without children by default, but you can overwrite this behaviour with the removeChildren parameter.
+ */
+export function makeJSXElementSelfClosing(
+  node: JSXElement,
+  context: Rule.RuleContext,
+  fixer: Rule.RuleFixer,
+  removeChildren: boolean = false
+): Rule.Fix[] {
+  if (!node.closingElement) {
+    return [];
+  }
+
+  const fixes = [];
+
+  const emptyChildren = childrenIsEmpty(node.children);
+
+  if (removeChildren || emptyChildren) {
+    const closingSymbol = context
+      .getSourceCode()
+      .getLastToken(node.openingElement)!;
+
+    fixes.push(
+      fixer.replaceText(closingSymbol, " />"),
+      fixer.remove(node.closingElement)
+    );
+
+    if (node.children.length) {
+      if (removeChildren) {
+        fixes.push(
+          fixer.removeRange([
+            node.children[0].range![0],
+            node.children[node.children.length - 1].range![1],
+          ])
+        );
+      } else if (emptyChildren) {
+        fixes.push(fixer.remove(node.children[0] as JSXText));
+      }
+    }
+  }
+
+  return fixes;
+}

packages/eslint-plugin-pf-codemods/src/rules/v6/buttonMoveIconsIconProp/button-moveIcons-icon-prop.test.ts

@@ -17,7 +17,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
   invalid: [
     {
       code: `import { Button } from '@patternfly/react-core'; const icon = <span>Some icon</span>; <Button variant="plain">{icon}</Button>`,
-      output: `import { Button } from '@patternfly/react-core'; const icon = <span>Some icon</span>; <Button icon={icon} variant="plain"></Button>`,
+      output: `import { Button } from '@patternfly/react-core'; const icon = <span>Some icon</span>; <Button icon={icon} variant="plain" />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -27,7 +27,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
     },
     {
       code: `import { Button } from '@patternfly/react-core'; <Button variant="plain"><span>Some icon</span></Button>`,
-      output: `import { Button } from '@patternfly/react-core'; <Button icon={<span>Some icon</span>} variant="plain"></Button>`,
+      output: `import { Button } from '@patternfly/react-core'; <Button icon={<span>Some icon</span>} variant="plain" />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -37,7 +37,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
     },
     {
       code: `import { Button, ButtonVariant } from '@patternfly/react-core'; <Button variant={ButtonVariant.plain}><span>Some icon</span></Button>`,
-      output: `import { Button, ButtonVariant } from '@patternfly/react-core'; <Button icon={<span>Some icon</span>} variant={ButtonVariant.plain}></Button>`,
+      output: `import { Button, ButtonVariant } from '@patternfly/react-core'; <Button icon={<span>Some icon</span>} variant={ButtonVariant.plain} />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -47,7 +47,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
     },
     {
       code: `import { Button as CustomThing } from '@patternfly/react-core'; <CustomThing variant="plain"><span>Some icon</span></CustomThing>`,
-      output: `import { Button as CustomThing } from '@patternfly/react-core'; <CustomThing icon={<span>Some icon</span>} variant="plain"></CustomThing>`,
+      output: `import { Button as CustomThing } from '@patternfly/react-core'; <CustomThing icon={<span>Some icon</span>} variant="plain" />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -57,7 +57,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
     },
     {
       code: `import { Button } from '@patternfly/react-core/dist/esm/components/Button/index.js'; <Button variant="plain"><span>Some icon</span></Button>`,
-      output: `import { Button } from '@patternfly/react-core/dist/esm/components/Button/index.js'; <Button icon={<span>Some icon</span>} variant="plain"></Button>`,
+      output: `import { Button } from '@patternfly/react-core/dist/esm/components/Button/index.js'; <Button icon={<span>Some icon</span>} variant="plain" />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -67,7 +67,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
     },
     {
       code: `import { Button } from '@patternfly/react-core/dist/js/components/Button/index.js'; <Button variant="plain"><span>Some icon</span></Button>`,
-      output: `import { Button } from '@patternfly/react-core/dist/js/components/Button/index.js'; <Button icon={<span>Some icon</span>} variant="plain"></Button>`,
+      output: `import { Button } from '@patternfly/react-core/dist/js/components/Button/index.js'; <Button icon={<span>Some icon</span>} variant="plain" />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -77,7 +77,7 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
     },
     {
       code: `import { Button } from '@patternfly/react-core/dist/dynamic/components/Button/index.js'; <Button variant="plain"><span>Some icon</span></Button>`,
-      output: `import { Button } from '@patternfly/react-core/dist/dynamic/components/Button/index.js'; <Button icon={<span>Some icon</span>} variant="plain"></Button>`,
+      output: `import { Button } from '@patternfly/react-core/dist/dynamic/components/Button/index.js'; <Button icon={<span>Some icon</span>} variant="plain" />`,
       errors: [
         {
           message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
@@ -96,6 +96,17 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
         },
       ],
     },
+    // with Icon component child (aliased)
+    {
+      code: `import { Button, Icon as PFIcon } from '@patternfly/react-core'; <Button><PFIcon>Some icon</PFIcon></Button>`,
+      output: `import { Button, Icon as PFIcon } from '@patternfly/react-core'; <Button icon={<PFIcon>Some icon</PFIcon>}></Button>`,
+      errors: [
+        {
+          message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
+          type: "JSXElement",
+        },
+      ],
+    },
     // with react-icons icon child
     {
       code: `import { Button } from '@patternfly/react-core'; import { SomeIcon } from "@patternfly/react-icons"; <Button><SomeIcon /></Button>`,
@@ -129,5 +140,27 @@ ruleTester.run("button-moveIcons-icon-prop", rule, {
         },
       ],
     },
+    // with react-icons icon child in children prop (inside JSXFragment)
+    {
+      code: `import { Button } from '@patternfly/react-core'; import { SomeIcon } from "@patternfly/react-icons"; <Button children={<>Text <SomeIcon /></>} />`,
+      output: `import { Button } from '@patternfly/react-core'; import { SomeIcon } from "@patternfly/react-icons"; <Button icon={<SomeIcon />} children={<>Text </>} />`,
+      errors: [
+        {
+          message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
+          type: "JSXElement",
+        },
+      ],
+    },
+    // with react-icons icon child in children prop (inside JSXElement)
+    {
+      code: `import { Button } from '@patternfly/react-core'; import { SomeIcon } from "@patternfly/react-icons"; <Button children={<span>Text <SomeIcon /></span>} />`,
+      output: `import { Button } from '@patternfly/react-core'; import { SomeIcon } from "@patternfly/react-icons"; <Button icon={<SomeIcon />} children={<span>Text </span>} />`,
+      errors: [
+        {
+          message: `Icons must now be passed to the \`icon\` prop of Button instead of as children. If you are passing anything other than an icon as children, ignore this rule when running fixes.`,
+          type: "JSXElement",
+        },
+      ],
+    },
   ],
 });

packages/eslint-plugin-pf-codemods/src/rules/v6/buttonMoveIconsIconProp/button-moveIcons-icon-prop.ts

@@ -1,13 +1,15 @@
 import { Rule } from "eslint";
-import { JSXElement } from "estree-jsx";
+import { JSXElement, JSXFragment } from "estree-jsx";
 import {
+  childrenIsEmpty,
   getFromPackage,
   getAttribute,
   getAttributeValue,
   getExpression,
   getChildrenAsAttributeValueText,
   getChildJSXElementByName,
   isReactIcon,
+  makeJSXElementSelfClosing,
 } from "../../helpers";
 
 // https://github.com/patternfly/patternfly-react/pull/10663
@@ -23,7 +25,7 @@ module.exports = {
     const buttonVariantEnumImport = imports.find(
       (specifier) => specifier.imported.name === "ButtonVariant"
     );
-    const hasIconImport = imports.some(
+    const iconImport = imports.find(
       (specifier) => specifier.imported.name === "Icon"
     );
 
@@ -53,31 +55,45 @@ module.exports = {
 
               const isPlain = variantValue === "plain" || isEnumValuePlain;
 
+              let plainButtonChildrenString: string | undefined;
+              let nodeWithChildren: JSXElement | JSXFragment = node;
               const childrenProp = getAttribute(node, "children");
 
-              let childrenValue: string | undefined;
-              if (childrenProp) {
+              if (childrenProp && childrenIsEmpty(node.children)) {
                 const childrenPropExpression = getExpression(
                   childrenProp?.value
                 );
-                childrenValue = childrenPropExpression
-                  ? `{${source.getText(childrenPropExpression)}}`
-                  : "";
+                if (
+                  childrenPropExpression?.type === "JSXElement" ||
+                  childrenPropExpression?.type === "JSXFragment"
+                ) {
+                  nodeWithChildren = childrenPropExpression;
+                }
+
+                if (isPlain) {
+                  plainButtonChildrenString = childrenPropExpression
+                    ? `{${source.getText(childrenPropExpression)}}`
+                    : "";
+                }
               } else if (isPlain) {
-                childrenValue = getChildrenAsAttributeValueText(
+                plainButtonChildrenString = getChildrenAsAttributeValueText(
                   context,
                   node.children
                 );
               }
 
-              if (!childrenValue && isPlain) {
+              if (!plainButtonChildrenString && isPlain) {
                 return;
               }
 
               const iconComponentChild =
-                hasIconImport && getChildJSXElementByName(node, "Icon");
+                iconImport &&
+                getChildJSXElementByName(
+                  nodeWithChildren,
+                  iconImport.local.name
+                );
 
-              const jsxElementChildren = node.children.filter(
+              const jsxElementChildren = nodeWithChildren.children.filter(
                 (child) => child.type === "JSXElement"
               ) as JSXElement[];
               const reactIconChild = jsxElementChildren.find((child) =>
@@ -100,17 +116,17 @@ module.exports = {
                   fixes.push(
                     fixer.insertTextAfter(
                       node.openingElement.name,
-                      ` icon=${childrenValue || iconChildString}`
+                      ` icon=${plainButtonChildrenString || iconChildString}`
                     )
                   );
 
-                  if (childrenProp) {
-                    fixes.push(fixer.remove(childrenProp));
-                  } else if (isPlain) {
-                    node.children.forEach(
-                      (child) =>
-                        child.type !== "JSXSpreadChild" &&
-                        fixes.push(fixer.replaceText(child, ""))
+                  if (isPlain) {
+                    if (childrenProp) {
+                      fixes.push(fixer.remove(childrenProp));
+                    }
+
+                    fixes.push(
+                      ...makeJSXElementSelfClosing(node, context, fixer, true)
                     );
                   } else if (iconChild) {
                     fixes.push(fixer.replaceText(iconChild, ""));