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

fix: type getVariableValue helper #796

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
JSXFragment,
JSXOpeningElement,
MemberExpression,
Property,
} from "estree-jsx";

export function getAttribute(
Expand Down Expand Up @@ -58,7 +59,7 @@ export function getAttributeValue(
return getVariableValue(node.expression.name, variableScope, context);
}
if (node.expression.type === "MemberExpression") {
return getMemberExpression(node.expression);
return node.expression;
}
if (node.expression.type === "Literal") {
return node.expression.value;
Expand All @@ -78,15 +79,6 @@ export function getExpression(node?: JSXAttribute["value"]) {
}
}

function getMemberExpression(node: MemberExpression) {
if (!node) {
return;
}
const { object, property } = node;

return { object, property };
}

export function getVariableDeclaration(
name: string,
scope: Scope.Scope | null
Expand Down Expand Up @@ -128,12 +120,12 @@ export function getVariableValue(
);
}
if (variableInit.type === "Literal") {
return variableInit.value;
return variableInit.value as string;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
if (variableInit.type === "MemberExpression") {
return getMemberExpression(variableInit);
return variableInit as MemberExpression;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
if (variableInit.type === "ObjectExpression") {
return variableInit.properties;
return variableInit.properties as Property[];
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { Rule } from "eslint";
import { Identifier, MemberExpression } from "estree-jsx";
import { getVariableDeclaration } from ".";

/** Used to get a property name on an enum (MemberExpression). */
export function getEnumPropertyName(
context: Rule.RuleContext,
enumNode: MemberExpression
) {
const isIdentifier = enumNode.property.type === "Identifier";
const computed = enumNode.computed;

// E.g. const key = "key"; someEnum[key]
if (isIdentifier && computed) {
const scope = context.getSourceCode().getScope(enumNode);
const propertyName = (enumNode.property as Identifier).name;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
const propertyVariable = getVariableDeclaration(propertyName, scope);
return propertyVariable?.defs[0].node.init.value as string;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
// E.g. someEnum.key
if (isIdentifier && !computed) {
return (enumNode.property as Identifier).name;
}
// E.g. someEnum["key"]
if (enumNode.property.type === "Literal") {
return enumNode.property.value as string;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Rule } from "eslint";
import { Property, Identifier } from "estree-jsx";
import { getVariableDeclaration } from "./JSXAttributes";
import { Property } from "estree-jsx";
import { propertyNameMatches } from "./propertyNameMatches";

/** Can be used to run logic on the specified property of an ObjectExpression or
* only if the specified property exists.
Expand All @@ -14,26 +14,9 @@ export function getObjectProperty(
return;
}

const matchedProperty = properties.find((property) => {
const isIdentifier = property.key.type === "Identifier";
const { computed } = property;

// E.g. const key = "key"; {[key]: value}
if (isIdentifier && computed) {
const scope = context.getSourceCode().getScope(property);
const propertyName = (property.key as Identifier).name;
const propertyVariable = getVariableDeclaration(propertyName, scope);
return propertyVariable?.defs[0].node.init.value === name;
}
// E.g. {key: value}
if (isIdentifier && !computed) {
return (property.key as Identifier).name === name;
}
// E.g. {"key": value} or {["key"]: value}
if (property.key.type === "Literal") {
return property.key.value === name;
}
});
const matchedProperty = properties.find((property) =>
propertyNameMatches(context, property.key, property.computed, name)
);

return matchedProperty;
}
3 changes: 3 additions & 0 deletions packages/eslint-plugin-pf-codemods/src/rules/helpers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export * from "./getChildJSXElementByName";
export * from "./getCodeModDataTag";
export * from "./getComponentImportName";
export * from "./getEndRange";
export * from "./getEnumPropertyName";
export * from "./getFromPackage";
export * from "./getImportedName";
export * from "./getImportPath";
Expand All @@ -22,12 +23,14 @@ export * from "./helpers";
export * from "./importAndExport";
export * from "./includesImport";
export * from "./interfaces";
export * from "./isEnumValue";
export * from "./isReactIcon";
export * from "./JSXAttributes";
export * from "./makeJSXElementSelfClosing";
export * from "./nodeMatches/checkMatchingImportDeclaration";
export * from "./nodeMatches/checkMatchingJSXOpeningElement";
export * from "./pfPackageMatches";
export * from "./propertyNameMatches";
export * from "./removeElement";
export * from "./removeEmptyLineAfter";
export * from "./removePropertiesFromObjectExpression";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { MemberExpression } from "estree-jsx";
import { propertyNameMatches } from "./propertyNameMatches";
import { Rule } from "eslint";

/** Checks whether an enum node (MemberExpression), e.g. ButtonVariant["primary"]
* has a given enumName ("ButtonVariant") and a given propertyName ("primary"), or one of given property names. */
export function isEnumValue(
context: Rule.RuleContext,
enumExpression: MemberExpression,
enumName: string,
propertyName: string | string[]
) {
if (
enumExpression?.object?.type === "Identifier" &&
enumExpression?.object?.name === enumName
) {
const nameMatches = (name: string) =>
propertyNameMatches(
context,
enumExpression.property,
enumExpression.computed,
name
);

if (Array.isArray(propertyName)) {
return propertyName.some((name) => nameMatches(name));
}

return nameMatches(propertyName);
}

return false;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { Rule } from "eslint";
import { Expression, Identifier, PrivateIdentifier } from "estree-jsx";
import { getVariableDeclaration } from "./JSXAttributes";

/** Check whether a property name is of a given value.
* Property can either be of an ObjectExpression - {propName: "value"} or MemberExpression - someObject.propName */
export function propertyNameMatches(
context: Rule.RuleContext,
key: Expression | PrivateIdentifier,
computed: boolean,
name: string
) {
const isIdentifier = key.type === "Identifier";

// E.g. const key = "key"; {[key]: value}; someObject[key]
if (isIdentifier && computed) {
const scope = context.getSourceCode().getScope(key);
const propertyName = (key as Identifier).name;
const propertyVariable = getVariableDeclaration(propertyName, scope);
return propertyVariable?.defs[0].node.init.value === name;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
// E.g. {key: value}; someObject.key
if (isIdentifier && !computed) {
return (key as Identifier).name === name;
wise-king-sullyman marked this conversation as resolved.
Show resolved Hide resolved
}
// E.g. {"key": value} or {["key"]: value}; someObject["key"]
if (key.type === "Literal") {
return key.value === name;
}

return false;
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Rule } from "eslint";
import { JSXElement, JSXFragment } from "estree-jsx";
import { JSXElement, JSXFragment, MemberExpression } from "estree-jsx";
import {
childrenIsEmpty,
getFromPackage,
Expand All @@ -10,6 +10,8 @@ import {
getChildJSXElementByName,
isReactIcon,
makeJSXElementSelfClosing,
propertyNameMatches,
isEnumValue,
} from "../../helpers";

// https://github.com/patternfly/patternfly-react/pull/10663
Expand Down Expand Up @@ -47,11 +49,16 @@ module.exports = {
variantProp?.value
);

const variantValueAsEnum = variantValue as MemberExpression;

const isEnumValuePlain =
buttonVariantEnumImport &&
variantValue?.object?.name ===
buttonVariantEnumImport.local.name &&
variantValue?.property.name === "plain";
!!buttonVariantEnumImport &&
isEnumValue(
context,
variantValueAsEnum,
buttonVariantEnumImport.local.name,
"plain"
);

const isPlain = variantValue === "plain" || isEnumValuePlain;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Rule } from "eslint";
import { JSXElement, Property, Literal } from "estree-jsx";
import { JSXElement, ObjectExpression, Property } from "estree-jsx";
import {
getAllImportsFromPackage,
getFromPackage,
checkMatchingJSXOpeningElement,
getAttribute,
Expand Down Expand Up @@ -54,19 +53,23 @@ module.exports = {
const selectableActionsValue = getAttributeValue(
context,
selectableActionsProp.value
);
) as ObjectExpression["properties"];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're having to cast the return values of getAttributeValue in mods now I feel like there's something not right going on.

I don't want to spam this comment out so echo this same thing for the below files where we're casting the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the casting is done because the actual selectableActions prop on CardHeader accepts only an object (CardHeaderSelectableActionsObject), so consumers shouldn't be able to pass anything else. I added a comment to make it clear.

The getAttributeValue should ultimately get to the ObjectExpression, or return undefined (e.g. if it was an Identifier imported from another file) in which case we don't do the fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yeah on many other places, the casting was done due to the lack of typing of the getAttributeValue helper

if (!selectableActionsValue) {
return;
}

const selectableActionsProperties = selectableActionsValue.filter(
(val) => val.type === "Property"
) as Property[];

const nameProperty = getObjectProperty(
context,
selectableActionsValue,
selectableActionsProperties,
"name"
);
const idProperty = getObjectProperty(
context,
selectableActionsValue,
selectableActionsProperties,
"selectableActionId"
);

Expand All @@ -92,7 +95,7 @@ module.exports = {
return [];
}
const propertiesToKeep = removePropertiesFromObjectExpression(
selectableActionsValue,
selectableActionsProperties,
validPropertiesToRemove
);
const replacementProperties = propertiesToKeep
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ module.exports = {
return;
}

const colorValue = getAttributeValue(context, colorProp.value);
const colorValue = getAttributeValue(
context,
colorProp.value
) as string;
if (Object.keys(replacementMap).includes(colorValue)) {
const newColorValue = replacementMap[colorValue];
const message = `The \`color\` prop on ${node.name.name} has been updated to replace "${colorValue}" with "${newColorValue}".`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@ module.exports = {
node,
"statusPageLinkText"
);
const statusPageLinkTextString: string =
getAttributeValue(context, statusPageLinkTextProp?.value) ??
"status page";
const statusPageLinkTextString =
(getAttributeValue(
context,
statusPageLinkTextProp?.value
) as string) ?? "status page";

if (statusPageLinkTextProp && statusPageLinkTextString.length) {
const firstChar = statusPageLinkTextString.charAt(0);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { Rule } from "eslint";
import { JSXOpeningElement } from "estree-jsx";
import { getFromPackage, getAttribute, getAttributeValue } from "../../helpers";
import { JSXOpeningElement, MemberExpression } from "estree-jsx";
import {
getFromPackage,
getAttribute,
getAttributeValue,
isEnumValue,
getEnumPropertyName,
} from "../../helpers";

// https://github.com/patternfly/patternfly-react/pull/10211
module.exports = {
Expand Down Expand Up @@ -38,18 +44,30 @@ module.exports = {
context,
colorVariantProp.value
);
const drawerColorVariantLocalName =
drawerColorVariantEnumImport &&
drawerColorVariantEnumImport.local.name;

const colorVariantValueAsEnum =
colorVariantValue as MemberExpression;

const hasPatternFlyEnum =
colorVariantValue &&
colorVariantValue.object &&
colorVariantValue.object.name === drawerColorVariantLocalName;
drawerColorVariantEnumImport &&
colorVariantValueAsEnum &&
colorVariantValueAsEnum.object &&
context
.getSourceCode()
.getText(colorVariantValueAsEnum.object) ===
drawerColorVariantEnumImport.local.name;

const isNoBackgroundEnum =
!!drawerColorVariantEnumImport &&
isEnumValue(
context,
colorVariantValueAsEnum,
drawerColorVariantEnumImport.local.name,
"noBackground"
);

const hasNoBackgroundValue =
colorVariantValue && colorVariantValue.property
? hasPatternFlyEnum &&
colorVariantValue.property.name === "noBackground"
: colorVariantValue === "no-background";
colorVariantValue === "no-background" || isNoBackgroundEnum;

if (!hasPatternFlyEnum && !hasNoBackgroundValue) {
return;
Expand All @@ -68,15 +86,19 @@ module.exports = {
}

if (!hasNoBackgroundValue && hasPatternFlyEnum) {
const enumPropertyName = colorVariantValue.property.name;
fixes.push(
fixer.replaceText(
colorVariantProp,
validDrawerContentValues.includes(enumPropertyName)
? `colorVariant="${colorVariantValue.property.name}"`
: ""
)
const enumPropertyName = getEnumPropertyName(
context,
colorVariantValueAsEnum
);
enumPropertyName &&
fixes.push(
fixer.replaceText(
colorVariantProp,
validDrawerContentValues.includes(enumPropertyName)
? `colorVariant="${enumPropertyName}"`
: ""
)
);
}
return fixes;
},
Expand Down
Loading
Loading