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-engine] Fix tagged template syntax with multiple expressions #23269

Merged
merged 8 commits into from
Oct 29, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 26, 2020

Assuming that @emotion/styled and @material-ui/core/styles#experimentalStyled implement the same API the added test fails unexpectedly.

Comparison between emotion and styled-engine: https://codesandbox.io/s/experimentalstyled-dynamic-styling-ubozu

Aside but not as severe: styled.div is not available. Only styled().

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 26, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 63b24bf

@oliviertassinari

This comment has been minimized.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 26, 2020

As far as I know, the CSS syntax is not really viable with JSS.

How does this PR relate to JSS? We're moving away from JSS to emotion and styled-components. If we don't implement the full interface of either one of those then we should document it.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 26, 2020

Double-checked I'm not relying on some obscure syntax and the advanced section of styled-components uses this syntax extensively: https://styled-components.com/docs/advanced

@mnajdova
Copy link
Member

I can confirm that this is an issue - https://codesandbox.io/s/quirky-hoover-yyyw3?file=/src/App.js for some reason I was always testing this syntax - https://codesandbox.io/s/goofy-lewin-gjbf4?file=/src/App.js which is working everywhere. Will take a look on this tomorrow, may be related to how I am adding the additional styles

@eps1lon
Copy link
Member Author

eps1lon commented Oct 26, 2020

Will take a look on this tomorrow, may be related to how I am adding the additional styles

That was my suspicion as well. I don't think we should maintain that "template-literal-mutation" approach. There's a reason why the raw strings are frozen. I looked a few days ago and it might make sense to use the css utility to pre-process the given styles and then append the resolved styles from the theme. The question is if that works with styled-components.

@mnajdova
Copy link
Member

I found out that I broke this functionality with #23205 when I was trying to solve the warning that comes from emotion. I reverted those changes and now it works with both emotion & styled-components, but we can still see the warning that we had before and it is still fragile..

That was my suspicion as well. I don't think we should maintain that "template-literal-mutation" approach. There's a reason why the raw strings are frozen. I looked a few days ago and it might make sense to use the css utility to pre-process the given styles and then append the resolved styles from the theme. The question is if that works with styled-components.

I agree, the css utility sounds like the perfect solution, but I can't see how that would work with styled-components. Will keep with the investigation and will post my findings here...

@mnajdova
Copy link
Member

Alright, I think I finally understood how the tagged templates work and how to add our own theme overrides, variants, sx props styles etc. All tests are passing, no warnings are shown and I've tested the dynamic props both with emotion and styled-components.

The first argument in the tagged template functions are the strings that represent the non dynamic parts of the template, while all other arguments are expressions that fall into one of those placeholders. What we need to do is, if the arguments are the string templates, we need to extend the first argument array to accept three additional values (overrides, variants, sx prop). Than we can safely add these in the rest of the arguments and again invoke the default styled function with the extended first argument, and the extended expressions list.

Very simplified version of the method:

const muiStyledResolver = (styleArg, ...expressions) => {
    
    let transformedStyleArg = styleArg;

    if (Array.isArray(styleArg)) {
      // If the type is array, than we need to add placeholders in the template for the overrides, variants and the sx styles
      transformedStyleArg = [...styleArg, '', '', ''];
      transformedStyleArg.raw = [...styleArg.raw, '', '', '']; // this is required prop for the tagged templates
    } else if (typeof styleArg === 'function') { // here we are just adding the defaultTheme in case the style is function not template
      // If the type is function, we need to define the default theme
      transformedStyleArg = ({ theme: themeInput, ...rest }) =>
        styleArg({ theme: isEmpty(themeInput) ? defaultTheme : themeInput, ...rest });
    }

    expressions.add(getOverridesfromTheme());// overrides from theme
    expressions.add(getVariantsfromTheme());// variants from theme
    expressions.add(getSxPropValue());// resolved styles from sx prop

    return defaultStyledResolver(transformedStyleArg, ...expressionsWithDefaultTheme);
  };

The experimentalStyled is basically doing the same, except that in additional it adds default theme in all expressions.. @eps1lon let me know if this makes sense, happy to add more tests and discuss details.

@mnajdova
Copy link
Member

I've duplicated each test to test both the string literal as well as the object/function syntax.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 27, 2020

Alright, I think I finally understood how the tagged templates work and how to add our own theme overrides

You're ahead of me there 😁

we need to extend the first argument array to accept three additional values (overrides, variants, sx prop).

Was about to post about the first argument being frozen but your implementation makes a copy instead.

I'll take a closer look at it tomorrow.

@eps1lon eps1lon changed the title [test] experimentalStyled and dynamic styling [styled-engine] Fix tagged template syntax with multiple expressions Oct 28, 2020
Copy link
Member Author

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Can't approve my own pull request but great work @mnajdova 👍

Just removed some unnecessary before setup. These shared variables tend to grow over time until it's hard to reason about a single test.


if (Array.isArray(styleArg)) {
// If the type is array, than we need to add placeholders in the template for the overrides, variants and the sx styles
transformedStyleArg = [...styleArg, '', '', ''];
Copy link
Member Author

Choose a reason for hiding this comment

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

This was my main point of interest since transpiling this results in quite a bit of babel helpers if array-spread isn't supported. But that's only the case for our /legacy bundle in v5 🎉

Though my the micro-perf part of my brain does want to dive into some v8 internals and see what's better 😁

-[...styleArg, '']
+styleArg.concat('')

Copy link
Member

Choose a reason for hiding this comment

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

In my mind, when I see

[...styleArg, '']

I am able to immediately conclude that this is a copy of the array with additional elements, for every other syntax I would need to take a closer look to decide whether it's a copy or mutation :) Of course this is probably just my preference, but you are probably right that it requires more advanced transpiling behind the scene :D

@eps1lon eps1lon merged commit a7459f6 into mui:next Oct 29, 2020
@eps1lon eps1lon deleted the test/styled-api branch October 29, 2020 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants