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

Styled components convert function with multiple return options to a static value #1794

Open
ochibikov-atlassian opened this issue Feb 13, 2025 · 0 comments

Comments

@ochibikov-atlassian
Copy link

ochibikov-atlassian commented Feb 13, 2025

Describe the bug
Dynamic properties don't work for the styled components when the first return value is a static string

To Reproduce
Steps to reproduce the behavior:

Create a component and render it anywhere (in unit tests/storybook/real website)

const component = styled.li({
	boxShadow: (props) => {
		if (props.isSelected) {
			return '0 0 0 1px magenta';
		}
		
		return '0 0 0 1px green';
	},
});

in this case boxShadow is always rendered with magenta color regardless if props.isSelected value. The function for the boxShadow isn't executed at runtime at all.

However, the function works properly and is executed at runtime when the first return statement contains a dynamic expression

boxShadow: (props) => {
	if (props.isSelected) {
		return `0 0 0 1px ${colors.N30}`;
	}
	return `1px 1px 2px -1px green`;
}

Expected behavior
If a property contains a function with conditional logic that might return different values, this logic should be retained at runtime, so that the rendered component has the correct css based on its current state.

Screenshots
When the first return contains a static string value the component always renders the magenta (regardless of the value in the props.isSelected that is checked in the condition):

Image

When the first return contains a dynamic value (a string template), then the function is executed at runtime and returns the correct value based on isSelected property (green in the code example above)

Image

Desktop (please complete the following information):

  • OS: [e.g. iOS] MacOS
  • Browser [e.g. chrome, safari] Chrome
  • Version [e.g. 22] 133.0.6943.54

Additional context
This happens in all environments including unit tests/storybooks/cypress/production website

The issue can be mitigated by converting a function to a ternary operator or making the first return statement dynamic using any other way:

boxShadow: (props) => {
	return props.isSelected ? '0 0 0 1px magenta' : '0 0 0 1px green';
},

However applying this workaround requires knowledge about the bug, without this knowledge the code with multiple returns seems totally valid, and it would lead to unexpected bugs.

The bug can be reproduced by adding the following unit tests to the babel plugin:

This test passes

  it('retains the dynamic behavior in the generated component when the property is a function where first return is a dynamic string', () => {
    const actual = transform(`
      import { styled } from '@compiled/react';
      import { colors } from 'a';
  
      const MyDiv = styled.div({
        boxShadow: (props) => {
          if (props.invalid) {
            return \`0 0 0 1px \${colors.red} \`;
          }
          return \`1px 1px 2px -1px black\`;
        },
      });
  
      export default () => <MyDiv />;
    `);

    expect(actual).toMatchInlineSnapshot(`
      "import{forwardRef}from'react';import*as React from'react';import{ax,ix,CC,CS}from"@compiled/react/runtime";import{colors}from'a';const _="._16qse35o{box-shadow:var(--_2p4m8a)}";const MyDiv=forwardRef(({as:C="div",style:__cmpls,...__cmplp},__cmplr)=>{if(__cmplp.innerRef){throw new Error("Please use 'ref' instead of 'innerRef'.");}const{invalid,...__cmpldp}=__cmplp;return<CC>
              <CS>{[_]}</CS>
              <C{...__cmpldp}style={{...__cmpls,"--_2p4m8a":ix((()=>{if(__cmplp.invalid){return\`0 0 0 1px \${colors.red} \`;}return\`1px 1px 2px -1px black\`;})())}}ref={__cmplr}className={ax(["_16qse35o",__cmplp.className])}/>
            </CC>;});if(process.env.NODE_ENV!=='production'){MyDiv.displayName='MyDiv';}export default()=><MyDiv/>;"
    `);
  });

This test fails

  it('retains the dynamic behavior in the generated component when the property is a function where first return is a static string', () => {
    const actual = transform(`
      import { styled } from '@compiled/react';
      import { colors } from 'a';
  
      const MyDiv = styled.div({
        boxShadow: (props) => {
          if (props.invalid) {
            return \`0 0 0 1px red \`;
          }
          return \`1px 1px 2px -1px black\`;
        },
      });
  
      export default () => <MyDiv />;
    `);

    expect(actual).toMatchInlineSnapshot(`
      "import{forwardRef}from'react';import*as React from'react';import{ax,ix,CC,CS}from"@compiled/react/runtime";import{colors}from'a';const _="._16qsap9n{box-shadow:0 0 0 1px red}";const MyDiv=forwardRef(({as:C="div",style:__cmpls,...__cmplp},__cmplr)=>{if(__cmplp.innerRef){throw new Error("Please use 'ref' instead of 'innerRef'.");}const{invalid,...__cmpldp}=__cmplp;return<CC>
              <CS>{[_]}</CS>
              <C{...__cmpldp}style={{...__cmpls,"--_2p4m8a":ix((()=>{if(__cmplp.invalid){return\`0 0 0 1px red \`;}return\`1px 1px 2px -1px black\`;})())}}ref={__cmplr}className={ax(["_16qse35o",__cmplp.className])}/>
            </CC>;});if(process.env.NODE_ENV!=='production'){MyDiv.displayName='MyDiv';}export default()=><MyDiv/>;"
    `);
  });

Instead of a function it returns a static string

 "import{forwardRef}from'react';import*as React from'react';import{ax,ix,CC,CS}from"@compiled/react/runtime";import{colors}from'a';const _="._16qsap9n{box-shadow:0 0 0 1px red}";const MyDiv=forwardRef(({as:C="div",style:__cmpls,...__cmplp},__cmplr)=>{if(__cmplp.innerRef){throw new Error("Please use 'ref' instead of 'innerRef'.");}const{invalid,...__cmpldp}=__cmplp;return<CC>
              <CS>{[_]}</CS>
              <C{...__cmpldp}style={__cmpls}ref={__cmplr}className={ax(["_16qsap9n",__cmplp.className])}/>
            </CC>;});if(process.env.NODE_ENV!=='production'){MyDiv.displayName='MyDiv';}export default()=><MyDiv/>;"
    `
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant