-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
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: |
@dminkovsky Tested and reproduced, I'll revert the change, thank you for the heads up! |
👍 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... |
@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 |
Hi @mattcarlotta. Thanks for reviewing this.
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.
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.
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.
This isn't isn't related to FOUC? If you configure styled-components correctly, you shouldn't see FOUC in neither v4 nor v5. |
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> |
@dminkovsky I was able to mitigate your issue by using two different approaches (shown below). I've also added interpolated 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 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): |
Hi @mattcarlotta, Thanks again for looking into this.
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?
Yeah I've done that a lot, but it got tedius writing
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:
if I were to pass a
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.
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. |
@dminkovsky Thanks for clarifying. The problem seems to lie within how v5 expects interpolated properties to be functions. If I change 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 Updated example repo with recomposed examples. However, this still relies upon interpolated property functions. I've also included a Baseimport styled from "styled-components";
const Base = styled.div`
cursor: pointer;
touch-action: none;
`;
export default Base; Composedimport 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; Nestedimport 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): |
In my case |
@mattcarlotta Thanks for investigating further!
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.
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. |
@beautyfree only with v5 and not v4? |
@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. |
@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 :) |
this helped me resolve my problem styled-components/styled-components#2379 (comment) |
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. |
@mattcarlotta hey thanks again for identifying the |
@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:
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. |
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. |
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. |
@dminkovsky I do. I haven't experienced any sluggishness. Then again, I have a 16C/32T processor. Are you experiencing sluggishness from the |
I believe the cause is the |
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