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

[Examples] Update styled-components to v5 in with-styled-components #11765

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

mottox2
Copy link
Contributor

@mottox2 mottox2 commented Apr 9, 2020

I updated styled-components to v5 in exmaples/with-styled-components.
This update has no breaking changes, but require react@16.8(hooks) so I update also react and react-dom.

https://medium.com/styled-components/announcing-styled-components-v5-beast-mode-389747abd987

@mottox2 mottox2 changed the title Update styled-components to v5 Update styled-components to v5 in with-styled-components Apr 9, 2020
@lfades lfades changed the title Update styled-components to v5 in with-styled-components [Examples] Update styled-components to v5 in with-styled-components Apr 9, 2020
Copy link
Member

@lfades lfades left a comment

Choose a reason for hiding this comment

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

Thank you!

@dminkovsky
Copy link

dminkovsky commented Jul 3, 2020

v5 breaks SSR in many cases. Maybe this should be reverted? I killed a lot of time on this a while back, wouldn't want to encourage others to do the same.

See:
styled-components/styled-components#3026 (has simple example)
styled-components/styled-components#2962 (has more details)

@lfades
Copy link
Member

lfades commented Jul 3, 2020

@dminkovsky Tested and reproduced, I'll revert the change, thank you for the heads up!

@dminkovsky
Copy link

👍

I keep checking in with styled-components to see if they've fixed it so that I can upgrade to v5, but no luck still. It's like—is styled components dead??? Don't tons of people use SSR/Next.js? Even this issue with Typescript is resolved with v5, but no dice with that SSR production bug...

@mattcarlotta
Copy link
Contributor

mattcarlotta commented Jul 4, 2020

@dminkovsky I cloned your next-styled-broken-prod example, dropped down to v4.4.1 and saw the same result in development and in production (second button never rendered properly). In fact, the only time it did render the second button was when I used v5.1.1 in production. Are you sure this is an issue with v5 and not an issue with how you're extending/composing the styled component?

On a side note, I'm using v5 in my current project and didn't notice any FOUC. The only time I did was when I dropped down to v4.4.1. When I bumped it to v5.1.1, it seemed to have resolved?/reduced? the FOUC issue (was unable to replicate what I saw in v4.4.1 despite disabling cache and adding network throttling; on a side note, text isn't visible because I'm using a custom font imported into a GlobalStylesheet, but everything else is a styled component).

@dminkovsky
Copy link

dminkovsky commented Jul 4, 2020

Hi @mattcarlotta. Thanks for reviewing this.

I cloned your next-styled-broken-prod example, dropped down to v4.4.1 and saw the same result in development and in production (second button never rendered properly).

Actually, in my demo, there are supposed to be two different buttons. So with 4.4.1 it renders properly: you should see two different buttons, both in development and production.

In fact, the only time it did render the second button was when I used v5.1.1 in production

When it renders both buttons the same: that's the bug. The buttons should be rendered differently, in both dev and prod, because they are the product of a closure that customizes the component two different ways.

Are you sure this is an issue with v5 and not an issue with how you're extending/composing the styled component?

Yes, I think so. All I'm doing is closing over a component that I am creating. I should get two different buttons. Moreover, it works in v5 development, just not v5 production.

On a side note, I'm using v5 in my current project and didn't notice any FOUC.

This isn't isn't related to FOUC? If you configure styled-components correctly, you shouldn't see FOUC in neither v4 nor v5.

@mattcarlotta
Copy link
Contributor

mattcarlotta commented Jul 4, 2020

Ahhh, I see! Misunderstood the problem. That's strange... can this issue be replicated beyond HOCs? In my couple of years of using styled-components, I've never seen a component composed this way. Is there any advantage? Instead, a styled component is either interpolated like this example:

const A = ({ children, className }) => (
  <div className={className}>
    <h1>Test</h1>
    <h2>{children}</h2>
    <h3>Test</h3>
  </div>
);

const StyledA = styled(A)`
  display: block;
`;

const B = styled.div`
  ${StyledA} {
     display: inline-block;
  }
`;

const Example = () => (
  <B>
    <A>
      Test
    </A>
  </B>
);

or extended like this example:

const A = styled.div`
  display: block;
`;

const B = styled(A)`
  display: inline-block;
`; 

...etc

On second thought, have you tried just passing in the css prop directly within the extended styled component?

const Root = styled(Base)`
  ${({ css }) => css};
`;

<Root css="display: block;cursor: pointer;">Extended</Root>

@mattcarlotta
Copy link
Contributor

mattcarlotta commented Jul 4, 2020

@dminkovsky I was able to mitigate your issue by using two different approaches (shown below). I've also added interpolated css examples according to the styled documentation.

Working demo (using v5.1.1)

My questions for you would be: Why do you need to use a HOC? And why do you need to use the css helper, when you're not interpolating props within a styled-component function?


Approach 1:

import styled from "styled-components";

const Base = styled.div`
  cursor: pointer;
  touch-action: none;
`;

const Button1 = styled(Base)`
  ${({ css }) => css};
`;

<Button1 css="user-select: none;transform: translateX(-3px);display: inline-block;padding: 12px 20px;border: 1px solid #dcdcdc;background-color: #fff;border-radius: 40px;color: #222;font-size: 16px;line-height: 1.75;letter-spacing: 0.2px;">
   Button1
</Button1>

Approach 2:

import styled from "styled-components";

const Base = styled.div`
  cursor: pointer;
  touch-action: none;
`;

const Button2 = styled(Base)`
  display: block;
  height: 36px;
  padding: 10px 16px;
  font-size: 14px;
  font-weight: bold;
  height: 31px;
  width: 36px;
  position: relative;
  color: #025aa5;
`;

<Button2>Button2</Button2>

Results (in production):

Button1:

Button2:

@dminkovsky
Copy link

dminkovsky commented Jul 4, 2020

Hi @mattcarlotta,

Thanks again for looking into this.

can this issue be replicated beyond HOCs?

I don't know. I've only encountered it in the example I posted. I don't think I should have said HOC though... this is more like a function that builds the component and captures certain CSS passed in. HOC makes it sound more complicated than it is?

On second thought, have you tried just passing in the css prop directly within the extended styled component?

Yeah I've done that a lot, but it got tedius writing ${({ css }) => css}; all the time. I also am a big fan of styled's extending capability like

const A = styled.div``; 
const B = styled(A)``;

and do that all the time for simple things.

I use the form in this example, though, when I want abstract a component tree from its styling, to build various styled versions of a component: Let's say a component is like:

function Component({cssA, cssB}) {
  return (
    <A css={cssA}>              {/* <-- render tree now polluted with static props that don't "do" anything */}
      <B css={cssB} />
    </A>
  )
}
const A = styled.div`${({ css }) => css}`; // <-- this definitely got old for me quickly
const B = styled.div`${({ css }) => css}`;

const C = () => <Component cssA="" cssB="" /> // <-- but cssA and cssB are static, why pass them as props?

if I were to pass a css= prop to style <A> and <B> like in the example above, I found that I get a lot of props polluting the picture that are actual static constants. So instead I write:

function buildComponent({cssA, cssB}) {
  const A = styled.div`${cssA}`;
  const B = styled.div`${cssB}`;
  return function Component() {
    return (
      <A>
        <B />
      </A>
    );
  }
}

const Component = buildComponent({cssA: '', cssB: ''})

const C = () => <Component />

To me, this is a lot cleaner. It may not seem like a big save, but my experience has been like there are many props like this that are actually static, that don't depend on their environment, and can be passed in through closures/factories like this. Then I also move lots of static props to styled().attrs({}), which removes even more clutters and lets you see how props are really running the show.

And why do you need to use the css helper, when you're not interpolating props within a styled-component function?

Yes that's a good question. I do this for one big reason: so that Prettier knows to format the CSS. For GraphQL you can mark a template string with /* GraphQL */, but with css I am not aware of any such way to mark it for formatting. Also, I had the impression the babel plugin requires this, but now that I'm thinking about it, I'm not so sure about that.

@mattcarlotta
Copy link
Contributor

mattcarlotta commented Jul 4, 2020

@dminkovsky Thanks for clarifying. The problem seems to lie within how v5 expects interpolated properties to be functions. If I change ${css}; to a function: ${() => css};, then it works as expected. They most likely dropped support for interpolated string properties in v5 since most of the time interpolated properties will be functions returning strings: color: ${props => props.white ? "white" : "black"}; instead of custom HOC'd components returning extended styled components with an interpolated string of properties... (just a guess, but it could be a bug)

For example, this works in production:

function composeButton(css) {
  const Root = styled(Base)`
    ${() => css};
  `;
  return function ComposeButton(props) {
    return <Root {...props}>Compose</Root>;
  };
}

That said, I was able to achieve similar functionality to the above using a popular packaged called recompose. Unfortunately, it's a bit dated, so I placed some of the helper functions within the utils folder.

Updated example repo with recomposed examples. However, this still relies upon interpolated property functions.

I've also included a withStyles helper function to extend the composed component. What's nice is that these additional helpers allow you to add styles, merge props, and/or set a display name (by the way, your example exports an Anonymous display name). You can get even fancier if you take a look at some of the other recompose helpers.

Base

import styled from "styled-components";

const Base = styled.div`
  cursor: pointer;
  touch-action: none;
`;

export default Base;

Composed

import Base from "../Base";
import { compose, displayName, withProps, withStyles } from "../../utils";

const { setDisplayName } = displayName;

const StyledA = compose(
  setDisplayName("Composed Styled A"),
  withProps((props) => ({
    ...props,
    onClick: () => alert("hi"),
  })),
  withStyles(`
    padding: 20px;
    color: pink;
    border: 1px solid pink;
    border-radius: 4px;
    width: 20px;
  `)
)(Base);

const StyledB = compose(
  setDisplayName("Composed Styled B"),
  withProps((props) => ({
    ...props,
    onClick: () => alert("bye"),
  })),
  withStyles(`
    padding: 20px;
    color: green;
    border: 1px solid green;
    border-radius: 4px;
    width: 30px;
  `)
)(Base);

const RecomposedA = compose(
  setDisplayName("Recomposed A"),
  withStyles(`
    color: green;
    border-color: green;
  `)
)(StyledA);

const ComposedExample = () => (
  <>
    <StyledA>Hi</StyledA>
    <br />
    <RecomposedA>Hi</RecomposedA>
    <br />
    <StyledB>Bye</StyledB>
  </>
);

export default ComposedExample;

Nested

import styled from "styled-components";
import { nest } from "../../utils";

const Article = styled.article`
  tex-align: center;
`;

const Container = styled.section`
  width: 200px;
  padding: 24px;
  border: 1px solid blue;
`;

const Content = styled.p`
  background: blue;
  color: white;
  padding: 10px;
  text-align: center;
`;

const Post = nest(Article, Container, Content);

export default Post;

Results (in production):


@beautyfree
Copy link

beautyfree commented Jul 9, 2020

In my case theme is empty with SSR of dynamic route (for example [jobAlias].tsx)

@dminkovsky
Copy link

@mattcarlotta Thanks for investigating further!

The problem seems to lie within how v5 expects interpolated properties to be functions. If I change ${css}; to a function: ${() => css};, then it works as expected.

That's great. That's something I could work with. Right now it's still easier for me to stay at v4, but if I really need to move to v5, at least I can do that. Will be tough finding all the spots to change though.

They most likely dropped support for interpolated string properties in v5 ... (just a guess, but it could be a bug)

Given that the bill v5 as a non-breaking update, I think this would be considered a bug? Though I concede most people probably use theme functionality or other methods you mentioned to achieve what I'm doing.

@dminkovsky
Copy link

@beautyfree only with v5 and not v4?

@mattcarlotta
Copy link
Contributor

@dminkovsky I'm in the process of building a composable-styled-components package built on v5. When you have time, play around with the demo. Does it feel similar to what you're trying to achieve? Or does it need some tweaking? Let me know.

⚠️ Please note that this package is still in the alpha stages -- missing quite a few features, missing documentation, needs production-ready tests, not published to npm, etc. Therefore, not recommended to be used in a production environment.

@dminkovsky
Copy link

@mattcarlotta sorry Matt, I'm trying to spend a little time possible on this element of my project right now. Have little-to-no time at all. Hanging on by a thread :)

@beautyfree
Copy link

@beautyfree only with v5 and not v4?

this helped me resolve my problem styled-components/styled-components#2379 (comment)

@t0x1c123
Copy link

I got working SSR of styled components at "next": "9.1.7" and "styled-components": "5.1.0". If I upgrade next to higher version SSR of styled components stops working.

@dminkovsky
Copy link

@mattcarlotta hey thanks again for identifying the ${() => css}; workaround. Helped me upgrade to v5 without re-writing a lot of stuff.

@mattcarlotta
Copy link
Contributor

@dminkovsky No problem. Just be careful though: I found that in development there can still be warnings thrown: #3238. I've mostly abandoned styled-components and have been using @emotion/styled.

Reasons I'd recommend you eventually switch:

  • It doesn't require a custom _document
  • It's jest integration actually works
  • It functions exactly like styled-components, so all you need to do is swap your imports from styled-components to @emotion/styled.

I haven't tested it with HOCs, but I believe it should work as I don't think emotion's interpolations require functions. Definitely worth exploring if you have some time.

@dminkovsky
Copy link

Hey thanks a lot for that information as well. I've had the sense that styled-components was getting supplanted, had the sense it might be emotion, but wasn't quite sure and didn't have time to confirm. Thanks a confirming and providing those details. Great to hear that @emotion/styled exists: hopefully it will be easy to find or write a jscodeshift mod that replaces the imports with minimal changes.

@dminkovsky
Copy link

Hey @mattcarlotta quick follow up for you— do you use Typescript? If so, do you remember if there was a speedup when you switched from styled to emotion? The styled typedefs trigger some really slow paths in the TS compiler, it makes everything painfully sluggish. Wondering if that's the case with emotion, too. Thank you.

@mattcarlotta
Copy link
Contributor

@dminkovsky I do. I haven't experienced any sluggishness. Then again, I have a 16C/32T processor. Are you experiencing sluggishness from the node_modules types or from developer-defined types from a custom styled component?

@dminkovsky
Copy link

I believe the cause is the node_modules types. There have been some improvements, but it is really slow. To be honest, I am not sure it's styled components, it's been a long time since I've profiled it, so maybe I should do that first :)

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants