Destructuring in styled interpolation functions #403

pgmanutd · 2020-12-11T05:52:04Z

This closes #397

itsdouges · 2020-12-11T06:39:14Z

examples/styled-interpolation-function-behavior.tsx

  font-size: ${(props) => props.textSize}px;
  background-color: ${(props) => {
    return props.bgColor;
  }};
-  border: 5px ${(props) => props.borderStyle} black;
+  border: 5px ${({ borderStyle }) => borderStyle} black;


can we add these tests as well please 😄

({ borderStyle: bs }) => bs propz => propz.borderStyle

Looks little bit tricky. Damn. We have hardcoded props check as of now. propz is now dynamic 😅

yeah 😅

make it work, make it right, make it fast? hehe

if its a lot of effort to allow props to be dynamically named e.g. propz we can leave it for now. we can potentially force it to be called props and just rename any member expressions to props? clever workarounds.

Was lil bit tricky. Damn! Finally 😅

itsdouges · 2020-12-14T21:04:24Z

packages/babel-plugin/src/utils/ast-builders.tsx

+ * 1. `propz.loading` in `border-color: \${(propz) => (propz.loading ? colors.N100 : colors.N200)};`
+ * Outcome: It will replace `propz.loading` with `props.loading`.
+ *
+ * 2. `prop.notValidProp` in `border-color: \${(props) => (props.notValidProp ? colors.N100 : colors.N200)};`


what is the edge case here? that the arg is props but we're using prop?

in this case should we just do nothing? could we delete some of this code and get away with it?

oops..it should be props.notValidProp. Sorry 😅

itsdouges · 2020-12-14T21:35:21Z

packages/babel-plugin/src/styled/__tests__/object-literal.test.tsx

+    expect(actual).toIncludeMultiple([
+      '{as:C="span",style,isLoading,loading,...props}',
+      'isLoading?colors.N20:colors.N40',
+      'loading?colors.N50:colors.N10',


hmm i think this is smart (instead of using l you use loading) - because it gets around the potential of naming collisions

can we have one test that asserts that so the knowledge is passed onto the next developer?

🎉 yups sure

itsdouges · 2020-12-14T21:41:17Z

packages/babel-plugin/src/utils/ast-builders.tsx

@@ -145,6 +145,109 @@ const traverseStyledBinaryExpression = (node: t.BinaryExpression, nestedVisitor:
  return node;
 };

+/**
+ * Handles cases like:
+ * 1. `propz.loading` in `border-color: \${(propz) => (propz.loading ? colors.N100 : colors.N200)};`


nit: could we handle this in an enter() visitor instead? so the logic of "rename propz to props" only needs to function once - instead of needing to traverse up the parent

🤔 thinking about it i see we check for arrow function or binary expression on L273/L277. perhaps this is fine?

So we are already traversing down. Here we are making sure propz is coming from function params. We picked member expression property, traversed to make sure it is coming from function params, and renamed it. Earlier we hardcoded the check to props.

itsdouges · 2020-12-14T21:41:35Z

packages/babel-plugin/src/utils/ast-builders.tsx

+ * @param path MemberExpression path
+ * @param propsToDestructure Array which gets mutated with props which are not valid eg. props.notValidProp
+ */
+const handleMemberExpressionInStyledInterpolations = (


nit:

-handleMemberExpressionInStyledInterpolations +handleMemberExpressionInStyledInterpolation

itsdouges · 2020-12-14T21:41:52Z

packages/babel-plugin/src/utils/ast-builders.tsx

+ * @param path Identifier path
+ * @param propsToDestructure Array which gets mutated with props which extracted from destructuring
+ */
+const handleDestructuringInStyledInterpolations = (


nit:

-handleDestructuringInStyledInterpolations +handleDestructuringInStyledInterpolation

itsdouges · 2020-12-14T21:43:07Z

packages/babel-plugin/src/utils/ast-builders.tsx

+  const traversedUpFunctionPath = path.find((parentPath) => parentPath.isFunction());
+  const memberExpressionKeyName = memberExpressionKey.name;
+
+  const isMemberExpressionKeyNameEqualsFunctionParams =


nit: isMemberExpressionNameTheSameAsFunctionFirstParam?

itsdouges · 2020-12-14T21:44:09Z

packages/babel-plugin/src/utils/ast-builders.tsx

+      return;
+    }
+
+    if (!propsToDestructure.includes(memberExpressionValueName)) {


can we add some comments here explaining what we're doing

(if valid html attribute let it through - else destructure to prevent)

itsdouges · 2020-12-14T21:44:47Z

packages/babel-plugin/src/utils/ast-builders.tsx

+
+    if (isPropValid(memberExpressionValueName)) {
+      // Convert cases like propz.color to props.color
+      if (memberExpressionKeyName !== 'props') {


can we move 'props' to module scope and reuse it? or move to constants.tsx? 🤔

itsdouges · 2020-12-14T21:45:58Z

packages/babel-plugin/src/utils/ast-builders.tsx

+  path: NodePath<t.Identifier>,
+  propsToDestructure: string[]
+) => {
+  // We are not interested in parent object property like `({ loading: l }) => l`


is this comment wrong? im not following unfortunately. more context?

So in this statement, ({ loading: l }) => l when we are checking for Identifier, both : l and => l are identifiers and function is parent for both. We are not interested in modifying : l. We just need to modify => l to => loading. If we don't skip, => l will not be modified because we have modified : l earlier and second identifier is nowhere to be found inside function params.
Hope this makes sense 😅

ahhh 🤔

so why don't we add loading to props to de-structure here?

alternatively now that i think about it - should we be adding destructured props anyway? currently the behaviour should be

am i a valid HTML element? you will be passed down to the raw HTML element

am i not valid? you will be destructured

do we need to rework anything to support this behaviour? 🤔

itsdouges · 2020-12-14T21:48:00Z

packages/babel-plugin/src/utils/ast-builders.tsx

+ */
+const handleDestructuringInStyledInterpolations = (
+  path: NodePath<t.Identifier>,
+  propsToDestructure: string[]


instead of leaking props to destructure can we instead return any identifiers we've found that we want to destructure?

itsdouges · 2020-12-14T21:48:57Z

packages/babel-plugin/src/utils/ast-builders.tsx

+ */
+const handleMemberExpressionInStyledInterpolations = (
+  path: NodePath<t.MemberExpression>,
+  propsToDestructure: string[]


instead of leaking props to destructure can we instead return any identifiers we've found that we want to destructure?

itsdouges · 2020-12-14T21:51:37Z

packages/babel-plugin/src/utils/ast.tsx

+}: {
+  name: string;
+  node: t.Expression | undefined;
+  resolveFor?: 'key' | 'value';


what if we just returned both?

Yups we can 😃

Another thought. Here we are traversing until we hit dead end. We either need to search for key or value. I mean where we will break the recursion. When either key is found or value is found?

no worries then :)

itsdouges

hey! left some comments have a look :)

looking pretty great! biggest thoughts are:

reworking the need to pass through propsToDestructure, and
seeing if we can do something about needing to traverse back up the AST to find the parent function

for 2) its more of a thought than anything, if it makes the code shittier then let's not bother

:fingerguns:

pgmanutd · 2020-12-15T04:11:38Z

hey! left some comments have a look :)

looking pretty great! biggest thoughts are:

reworking the need to pass through propsToDestructure, and

seeing if we can do something about needing to traverse back up the AST to find the parent function

for 2) its more of a thought than anything, if it makes the code shittier then let's not bother

:fingerguns:

First is fine. Second one hmm 🤔. Problem is how we can find where it is coming from. Looks like scope won't work because parent for loading ? color1: color2 will be ConditionalExpression and not the function. We need destructuring key in cases like { loading: l }.

Resolved

itsdouges

lgtm

pgmanutd requested a review from itsdouges December 11, 2020 05:52

itsdouges reviewed Dec 11, 2020

View reviewed changes

pgmanutd force-pushed the destructuring-in-styled-interpolation-functions branch from a3add01 to 1a727b9 Compare December 14, 2020 15:30

pgmanutd requested a review from itsdouges December 14, 2020 15:33

itsdouges reviewed Dec 14, 2020

View reviewed changes

itsdouges previously requested changes Dec 14, 2020

View reviewed changes

pgmanutd added 4 commits December 15, 2020 10:25

fix(styled): add logic for handing destructuring

ef451ec

fix: storybook not picking latest babel plugin changes

c453d55

feat(styled): handle destructuring edge cases

e6d6ecd

chore: implement review comments

ebaca9a

pgmanutd force-pushed the destructuring-in-styled-interpolation-functions branch from 1a727b9 to ebaca9a Compare December 15, 2020 05:50

pgmanutd requested a review from itsdouges December 15, 2020 05:52

chore: implement review comments-2

e338120

pgmanutd added 2 commits December 15, 2020 12:20

test: make expects explicit

1182f5f

test: cleanup

d603944

itsdouges approved these changes Dec 15, 2020

View reviewed changes

pgmanutd merged commit 492aee7 into master Dec 15, 2020

pgmanutd deleted the destructuring-in-styled-interpolation-functions branch December 15, 2020 07:04

pgmanutd mentioned this pull request Dec 15, 2020

Destructuring in a style interpolation does not pass valid HTML attributes down to the DOM element #407

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destructuring in styled interpolation functions #403

Destructuring in styled interpolation functions #403

pgmanutd commented Dec 11, 2020

itsdouges Dec 11, 2020 •

edited

pgmanutd Dec 11, 2020

pgmanutd Dec 11, 2020

itsdouges Dec 12, 2020 •

edited

pgmanutd Dec 14, 2020

itsdouges Dec 14, 2020 •

edited

pgmanutd Dec 15, 2020

itsdouges Dec 14, 2020

pgmanutd Dec 15, 2020

itsdouges Dec 14, 2020

pgmanutd Dec 15, 2020 •

edited

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020 •

edited

pgmanutd Dec 15, 2020 •

edited

itsdouges Dec 15, 2020

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020

itsdouges Dec 14, 2020

pgmanutd Dec 15, 2020

pgmanutd Dec 15, 2020

itsdouges Dec 15, 2020

itsdouges left a comment

pgmanutd commented Dec 15, 2020 •

edited

itsdouges left a comment

Destructuring in styled interpolation functions #403

Destructuring in styled interpolation functions #403

Conversation

pgmanutd commented Dec 11, 2020

itsdouges Dec 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsdouges Dec 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsdouges Dec 14, 2020 • edited

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

pgmanutd Dec 15, 2020 • edited

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

itsdouges Dec 14, 2020 • edited

Choose a reason for hiding this comment

pgmanutd Dec 15, 2020 • edited

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

itsdouges left a comment

Choose a reason for hiding this comment

pgmanutd commented Dec 15, 2020 • edited

itsdouges left a comment

Choose a reason for hiding this comment

itsdouges Dec 11, 2020 •

edited

itsdouges Dec 12, 2020 •

edited

itsdouges Dec 14, 2020 •

edited

pgmanutd Dec 15, 2020 •

edited

itsdouges Dec 14, 2020 •

edited

pgmanutd Dec 15, 2020 •

edited

pgmanutd commented Dec 15, 2020 •

edited