fix: type getVariableValue helper #796

adamviktora · 2024-10-23T13:14:08Z

I had to add some new helpers to simplify proper type checking.

wise-king-sullyman · 2024-11-11T20:26:20Z

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

@@ -128,12 +120,12 @@ export function getVariableValue(
    );
  }
  if (variableInit.type === "Literal") {
-    return variableInit.value;
+    return variableInit.value as string;


I don't love this cast since it might not actually be a string.

Instead of casting it could we call toString on it or something if we want the returned value to always be a string?

Good point, we probably don't even need it to always be a string

wise-king-sullyman · 2024-11-11T20:27:56Z

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

  }
  if (variableInit.type === "MemberExpression") {
-    return getMemberExpression(variableInit);
+    return variableInit as MemberExpression;


And rather than this cast can we fix the typing of variableInit to resolve as any? That should make this implicitly type correctly.

wise-king-sullyman · 2024-11-11T20:28:24Z

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

  }
  if (variableInit.type === "ObjectExpression") {
-    return variableInit.properties;
+    return variableInit.properties as Property[];


I'm not sure about this cast either, looking through the type defs I think it could also be spread props no?

wise-king-sullyman · 2024-11-11T20:29:56Z

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

+  // E.g. const key = "key"; someEnum[key]
+  if (isIdentifier && computed) {
+    const scope = context.getSourceCode().getScope(enumNode);
+    const propertyName = (enumNode.property as Identifier).name;


Are the Identifier casting here and on line 22 really valid?

Yeah, it is valid because of the isIdentifier, but I'll rewrite it so the casting is not necessary

wise-king-sullyman · 2024-11-11T20:32:38Z

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

+    const scope = context.getSourceCode().getScope(enumNode);
+    const propertyName = (enumNode.property as Identifier).name;
+    const propertyVariable = getVariableDeclaration(propertyName, scope);
+    return propertyVariable?.defs[0].node.init.value as string;


The property accessing here looks pretty similar to the variableInit in the last file, may be worth having a helper for dealing with resolving the value of a variable declaration, or fixing the typing of that helper?

Also similar note to my other one about the string assertion.

wise-king-sullyman · 2024-11-11T20:33:04Z

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

+  }
+  // E.g. someEnum["key"]
+  if (enumNode.property.type === "Literal") {
+    return enumNode.property.value as string;


Same string type assertion concern here.

wise-king-sullyman · 2024-11-11T20:36:20Z

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

+    const propertyName = (key as Identifier).name;
+    const propertyVariable = getVariableDeclaration(propertyName, scope);
+    return propertyVariable?.defs[0].node.init.value === name;
+  }
+  // E.g. {key: value}; someObject.key
+  if (isIdentifier && !computed) {
+    return (key as Identifier).name === name;


Same type casting concerns here.

wise-king-sullyman · 2024-11-11T20:37:07Z

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

+    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;


And same thought about it maybe being worth us making a type safe helper for traversing propertyVariable

wise-king-sullyman · 2024-11-11T20:39:07Z

...-plugin-pf-codemods/src/rules/v6/cardUpdatedClickableMarkup/card-updated-clickable-markup.ts

@@ -54,19 +53,23 @@ module.exports = {
              const selectableActionsValue = getAttributeValue(
                context,
                selectableActionsProp.value
-              );
+              ) as ObjectExpression["properties"];


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.

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

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

wise-king-sullyman

Great work on this so far! But I feel like for an issue focused on improving our typing we probably don't want to be adding a bunch of type casting if it can at all be avoided.

adamviktora added 6 commits October 23, 2024 15:11

fix: type getVariableValue helper

0bb13f7

feat(helpers): isEnumValue and propertyNameMatches helpers

589c021

refactor(helpers)

a84ba10

feat(helpers): getEnumPropertyName helper

c43d8a6

feat(helpers): update isEnumValue helper to support list of strings

1d5507c

feat(misc): don't use "any" type

e9f8593

adamviktora force-pushed the getAttributeValue-typed branch from 9710972 to e9f8593 Compare October 30, 2024 10:47

adamviktora requested review from thatblindgeye and wise-king-sullyman October 30, 2024 10:52

wise-king-sullyman reviewed Nov 11, 2024

View reviewed changes

wise-king-sullyman requested changes Nov 11, 2024

View reviewed changes

adamviktora force-pushed the getAttributeValue-typed branch from ccfc9bd to f553135 Compare November 12, 2024 12:43

fix(misc): get rid of type casting / explain type casting

72f488f

adamviktora force-pushed the getAttributeValue-typed branch from f553135 to 72f488f Compare November 12, 2024 12:46

adamviktora linked an issue Nov 12, 2024 that may be closed by this pull request

getVariableValue helper: check that type of variableDeclaration.defs[0] === "Variable" #630

Open

adamviktora requested a review from wise-king-sullyman November 12, 2024 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: type getVariableValue helper #796

fix: type getVariableValue helper #796

adamviktora commented Oct 23, 2024 •

edited

Loading

wise-king-sullyman Nov 11, 2024

adamviktora Nov 12, 2024

wise-king-sullyman Nov 11, 2024

wise-king-sullyman Nov 11, 2024

wise-king-sullyman Nov 11, 2024

adamviktora Nov 12, 2024

wise-king-sullyman Nov 11, 2024

wise-king-sullyman Nov 11, 2024

wise-king-sullyman Nov 11, 2024

wise-king-sullyman Nov 11, 2024

wise-king-sullyman Nov 11, 2024

adamviktora Nov 12, 2024

adamviktora Nov 12, 2024

wise-king-sullyman left a comment

fix: type getVariableValue helper #796

Are you sure you want to change the base?

fix: type getVariableValue helper #796

Conversation

adamviktora commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wise-king-sullyman left a comment

Choose a reason for hiding this comment

adamviktora commented Oct 23, 2024 •

edited

Loading