Deprecate .extend in favour of only styled(Component)
pke opened this issue · 70 comments
The FAQ explains the difference between styled() and .extend. However I wonder if it would be possible to take the decision out of the devs hand and have styled check for the handed in Component if its already a styled component. If it is .extend it otherwise create a new class.
Was this considered at one point?
This was how it originally worked, actually—and honestly, I'm not sure it was the right decision to split them. This is the most common confusion for developers, so I think we should move back to the original version and deprecate .extend in the next major. (with a codemod, obviously)
@mxstbr Totally agreed! We've discussed removing it before—and there was some general concensus—so I think it's time to pull the trigger.
Shall we deprecate it in the next minor release?
Let's deprecate and remove from the docs in the next major and make sure we have a codemod in place, then remove the code entirely in the next major after that.
@mxstbr Do you have any reason to believe that we need to keep it around for another major version? 😅 I think that won't actually make a huge difference
I don't think folks will be particularly happy about us suddenly throwing warnings in a minor version upgrade.
Thanks for considering this simplification of the API surface. I agree such removal of API should not happen in a minor update. Please stay true to semver.
The behaviour of styled can be changed in a minor version but the deprecation warning could be introduced in the next major RC.
On the other hand why not release a new major version right away. What to wait for?
Yeah exactly.
Yea, I don't have a problem with issuing a major version asap. However, semver doesn't restrict deprecations to major releases. That'd be quite extreme as you'd then during a typical release flow have to release two major bumps—one potentially without breaking changes—just to get new APIs in and remove old ones.
Quote:
When you deprecate part of your public API, you should do two things: (1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.
But anyhow, I'm fine with both, but I'd like to coordinate a new release with #1493 and all related deprecations (injectGlobal). So I'd love to push out a major only when #1493 is done, and deprecate in a minor, but I'll leave the call up to you, @mxstbr 👍
Btw I'll rename the issue to represent the new plan.
We had a similar convo last year: #1087
I'm definitely in favor of getting rid of .extend from the public API.
@probablyup @mxstbr to reiterate a little, I'd wait until both aforementioned PRs are merged and available at which point we can safely deprecate .extend, injectGlobal, and keyframes (if an alternative will be available) in a minor. Then in the next major we can remove them.
But we could already start a pending PR that adds the deprecation warnings I suppose?
Sounds perfect
Reading #1087 I see back then there were strong arguments by members of the project against flattening the API. Good to see decisions can be reverted if they seem to confuse to many users (!) of the library even if the owners initially thought broadening the API surface was a good thing.
@pke to add some more depth to this prior discussion, what @probablyup was seeing became an increasingly large problem on the implementation side—kudos for seeing this so early btw—and while we preferred the API, over time it became clear that we didn’t even agree ourselves on its practical usage and use cases. Together with having problems with it (component IDs), and having no technical reason for it (specificity correctness is guaranteed, so why concat css?) this is proving to be the way forward.
Hope this dimes up everyone’s opinion on the matter :)
If I am following this correctly, moving forward styled(Fruit) would work how Fruit.extend does today?
const Fruit = styled('div')`
color: green;
`;
const Orange = styled(Fruit)`
background: orange;
`;const Fruit = styled('div')`
color: green;
`;
const Orange = Fruit.extend`
background: orange;
`;I'm all for the simplification, however it would be great if the original styles didn't have to be duplicated which is the same issue I have with how .withComponent() works. Obviously in my sample it’s not a big deal. However, if Fruit had tons of styles and needed to be extended many times, then it seems inefficient to duplicate the styles each time instead of just having these styles defined on a single CSS class.
How would you style a styled component created with withComponent? Currently I sometimes do
const Fruit = styled.div``
const Orange = Fruit.withComponent('span').extend``Will the following work instead?
const Orange = styled(Fruit.withComponent('span'))@denkristoffer yes.
hi, just to add my 2 cents.. I totally agree, this is indeed confusing when starting with styled-components. so i like the idea, as a consumer, not to think about what kind of components I am extending, but here it comes...
The documentation stated for a while i think? that .extend is the reccomended way.. but now the 'recommended' way will be deprecated?
Why not just leave..both.. so that it it doesnt matter wichone you choose. So, eliminating the confusion, YES, but leave the choice (personal flavour i would say) to use styled(Component) or Component.extend, even if it does the same. Personally i like the second version, because its natural to start thinking wich Component to use, and then what you want to do with it (thank you intellisense..)..
and you do not need the import styled from 'styled-components'..
If extend is deprecated and removed, will there by an analogous .attrs() for styled?
@ckeeney attrs() works with styled() as it is:
const A = styled(B).attrs({
style: ({ opacity }) => ({
opacity,
}),
})`
color: red;
`We should be careful here. extend has not the same behaviour as styled. In our case we grep the component displayName in our visual tests. There it makes a difference what we use. If we use extend, it will not get a new displayName, but with styled it does. E.g.:
Will result in:
@ms88privat apart from slightly different stack traces and component names, is there any practical difference here to regular users of styled-components?
@ckeeney Yes, a larger response size for server rendered sites since duplicate styles are defined instead of sharing the same CSS class.
If I understand correct, making styled(Foo) to be equal to Foo.extend if Foo is already a styled-component would seriously break one case. Which is a contextual usage of a component.
Example: having one template component for buttons: Button. Then we can create new versions of buttons by using NewButton = styled(Button) and get one important feature: whenever we would use ${Button} in selectors, they would target any buttons: both simple Button components and modified NewButton ones. This can be crucial for a lot of use cases.
I'm really against completely removing this feature. While I'm ok for making default behavior of styled(Foo) the same as for Foo.extend, and in that case, there would be a need to add an escape hatch for those who would need to have the current behavior.
@kizu a new stable classname is created for each individual styled component class, have no fear
The fear is that & > ${Button} inside styles of some other Foo component won't target a NewButton component.
For that case, I'd recommend tagging the base component with some well known class or attribute and referring to that. We don't formally support inheritance in component selectors.
e.g.
const BUTTON_CLS = 'button';
const Button = styled.button.attrs({ className: BUTTON_CLS })`
color: red;
`;
const OtherButton = styled(Button)`
color: blue;
`;
const Wrapper = styled.div`
.${BUTTON_CLS} {
background: green;
}
`;
<Wrapper>
<Button>Hi</Button>
<OtherButton>Hello</OtherButton>
</Wrapper>Both would have green backgrounds.
Sorry, but there is literally a section dedicated to this method in the documentation: https://www.styled-components.com/docs/advanced#referring-to-other-components, so it is strange to hear that this is something which is not “formally supported”.
As this is already in the doc, and people already use this method which documentation calls as allowing for “extremely powerful composition patterns”, this could be a really big hit to a lot of projects.
Ok, I understand what you're talking about, but still — right now this works this way, there are projects that rely on this feature and, yes, 4.0.0 could be breaking in this regards, but without a built-in escape hatch developers would need to cheat and make it so styled-components won't understand that you're passing an existing styled component to styled(). No one would change the existing behavior that just works and does the work perfectly to some classes that are not guaranteed to be unique.
Mixing multiple classes on one element is a really powerful feature and it is strange that it is not supported. Ok, it obviously has its drawbacks, and that is why I'm ok with the default behavior to be the same as extend.
Mixing multiple classes on one element is a really powerful feature and it is strange that it is not supported
What do you mean? styled-components let's you do whatever you want. It just generates a stable unique className per styled component class and appends it to the rendered DOM. That's what is used when composing a component selector.
The difference between Thing.extend and styled(Thing) is that Thing.extend makes a copy of all the styles of Thing and assigns a single new dynamic className for them.
If you're referring in code to the dynamic classNames generated by a styled-component and not the stable one (Thing.styledComponentId), I highly recommend avoiding that as it isn't part of the API of this library and could change at any time.
However this duplicates the styles so they are defined on separate class names instead of either sharing a single class name or having a selector that contains multiple class names (.a, .b { }). Has anyone studied the performance impact this has for both client and server rendered sites?
Yes, I'm acutely aware of how extend and styled() work. I'm not referring in code to the dynamic classNames, and I have already written that I use it per the docs I have mentioned — by using ${MyComponent} as a part of the selector.
The problem, again, is that the change in 4.0.0 makes styled-components to treat all other styled-components in a special way from any other components. It would dismiss the original components' stable className.
There are cases when we need both pairs of classNames: those from the original component, and the new ones. We need both stable classNames on one element.
I fell like I'm repeating myself.
This API has been removed in the v4.0 branch per the roadmap. We'll add a deprecation message to v3.x in an upcoming minor release.
Could the docs get updated too? I've been following https://www.styled-components.com/docs/faqs#which-one-should-you-use and have created unnecessary technical debt for my team.
@sdalonzo PR to the docs would be much welcome: https://github.com/styled-components/styled-components-website
I love the work that's done here, and deprecation warnings are important, but:
- The new warning is really spamming Jest test output. Can we somehow show it only once? I think that's how it works in the browser, but not in Jest.
Why useFixed inconsole.errorfor a warning? Is it for the stack trace (console.warnshows it too)? Is it for increased visibility? I believe those aren't good reasons. It should be a warning.styled-components@3.4.5, thanks to @probablyup !
Thanks again! 👍
@elektronik2k5 PRs fixing those issues welcome!
@elektronik2k5 I think it's because Jest resets the module registry regularly and when it requires the file fresh, the "once" shortcircuit gets reset since the whole function is recreated. Switching it to a warning sounds fine though.
Should I update the documentation with this deprecation in "extending styles" page? Because there is nothing about our old friend ".extend" (or I couldn't find it)
Also I wrote small regex to move it to new structure.
Search: (\w*.withComponent\('\w*'\)|\w*.|\w*.\w*.)(?:\.extend`)
Replace: styled($1)`
It works on vs code and atom.
this regex supports;
ComponentName.extend` ==> styled(ComponentName)`
ComponentName.withComponent('span').extend` ==> styled(ComponentName.withComponent('span'))`
ComponentName.ChildName.extend` ==> styled(ComponentName.ChildName)`
It doesn't support X.extend`...`.withComponent('div'); usages.
@Sly777 there is at lease one use case to watch out for if I remember things correctly:
const X2 = X.extend`...`.withComponent('div');should become
const X2 = styled(X.withComponent('div'))`...`;Aaa I see. I didn't know that usage way.
I'm seeing this warning pop up a lot. Is there a codemod for this?
@mxstbr is there is a need to write a codemod? I can write one with pleasure (babel-codemod) :)
@RIP21 please feel free! We'd then want to add a link to it in the FAQ section of the website
A codemod would be awesome!!!!
We'd then want to add a link to it in the FAQ section of the website
...and the release notes for v4 👍
If not mentioned, the codemod would also need to add the import to any files?
import styled from 'styled-components'So, codemod is here 🎉
https://github.com/RIP21/styled-components-v3-to-v4-codemod
Check this thing out.
Beware, babel-codemod is weirdly wraps async functions and some other expressions with () which doesn't change the execution of the code, but if you appeared to have issues with that aware that it's not related to the code of codemod itself but underlying tooling used in babel-codemod :) You may fill the issue there, but just sayin'
what does .extend do?
I found it in a cobase that I am new to.
I know you can do:
styled(SomeComponent)`
// The CSS that we want to add to SomeComponent, on top of the CSS that it already has, goes here.
`
I'm assuming that .extend does something similar but I can't find exactly what it does and I'm just guessing.
Now I see talks about how this will be deprecated and I'm just even more confused now.
Sorry if this has already been explained. But I have read almost have all the comments on this page and can't find the simple answer I am looking for. And can't find it in the docs either.
@Osuriel if you want to extend the styles of any component just wrap it in styled function. const RedComponent = styled(Component)`color: red;`
Extend is history now.
Maybe anyone can explain this issue to me:
const Button = styled.button`
// ... All basic styling
`
const CTAButton = styled(Button)`
//... Just override a few colors, etc.
`
const CTAButtonLink = CTAButton.withComponent('a');
And now my CTAButtonLink has only CTAButton styling, and lost all the Button styling.
With const CTAButton = Button.extend everything worked fine. What I'm doing wrong here and how could I fix that?
Thanks.
@ppozniak looks absolutely valid.
Try to reproduce that in codesandbox.io and open issue if it's the case.
I commented in the issue thread. The behavior is valid, just a misunderstanding of how the API works.
Is it still the plan to have styled(Component) detect if Component is a styled-component, and if so, apply the .extend logic under the hood?
We're getting the deprecation warnings, but we can't actually upgrade until the .extend functionality is restored under the hood of styled(), as was suggested in the original description of this issue.
Current compatibility issues with .withComponent are highlighted below:
const Button = styled.button`
// Imagine some styles here
`;
const ExtendedButton = Button.extend`
color: green;
`;
const StyledButton = styled(Button)`
color: green;
`;
// FAILS :(
const StyledButtonLink = styled(StyledButton.withComponent('a'))`
font-weight: bold;
`
// PASSES :) (because we used .extend)
const ExtendedButtonLink = styled(ExtendedButton.withComponent('a'))`
font-weight: bold;
`Here's a code pen demonstrating where this breaks down, and why we can't upgrade our app:
Hey @greypants, could you see the convo here? #1956
If you're using withComponent for the purpose of switching out what physical element is being rendered, a polymorphic component base makes much more sense.
@probablyup woaaaaaaah... that pattern is awesome, thanks!
So the answer then, is "No"? The .extend functionality is being removed completely, not moved back into styled().
Does that mean .withComponent should also be deprecated in favor of the polymorphic component pattern, since it's incompatible with styled()?
Does that mean .withComponent should also be deprecated in favor of the polymorphic component pattern, since it's incompatible with styled()?
We haven't made a formal decision on it yet, but probably. I'm doing some experimentation to see how difficult it would be to bring the polymorph functionality in as a first class prop API in styled
Hello,
It seems that without the .extend functionality, there is no attribute validation within styled-components.
So the following breaks:
it('should not adopt an invalid attribute', () => {
const renderedComponent = mount(<CompA attribute="foo" />);
expect(renderedComponent.prop('attribute')).toBeUndefined();
});
// compB is style('section')
it('should render an <section> tag', () => {
const renderedComponent = shallow(<CompA />);
expect(renderedComponent.type()).toEqual('section');
});
// Expected value to equal:
// "section"
// Received:
// [Function StyledComponent]
With the component:
const CompA = styled(CompB)`
height: 100%;
color: black;
`;
@RIP21 thanks so much for that codemod! I've moved it to the styled-components organization and packaged it into a single executable for nicer usage: https://github.com/styled-components/styled-components-codemods
Will publish a first beta version to npm to try it out 🎉
Apologies for posting here but I have tried to search through issues and the change log but would appreciate some clarification.
At what version did styled() get updated to deprecate .extend?
Obviously in v4 .extend is no more and styled() "folds" the functionality when trying to style a styled component.
I can see in v3.4.3 that a warning was added when using extend -- does that mean at this point styled() is handling styling styled-components the way extend used to?
We are using extend in a number of places, in apps with a range of versions (back to v2) and am unsure about when I can swap out our .extends in our shared components.
Thanks!
Hey @RIP21 Thanks for the quick reply 🙂
I am confused. Surely styled() can't work the exact same way it always has otherwise the requirement for .extend would still exist/never have existed?
In my experience using styled(SomeStyledComponent) results in multiple sc- classnames and styles being applied (e.g. sc-jtRfpW sfcle sc-elJkPf iJAdYA) with one bringing the original styles and then the other the "overrides". The issue that extend has always fixed for me was that the css for these classes can be in the wrong order. So your overrides appear before the default styles in the stylesheet and therefore do not override.
.extend _merges these styles into the one classname and avoids this whole source-order issue.
@lauriejones no, the refactor that "folds"/merges styles into each other was only released in v4, although I'm pretty sure your code should still work in v3 if changed to styled(Comp) from .extend.
I believe I'm running into the same issue that @lauriejones described. After upgrading and moving over to styled in v4, I'm running into specificity issues all over the place. It's hard to tell if it's because something extend was doing was masking actual issues, but it's certainly frustrating to debug.
Is it still the plan to have
styled(Component)detect ifComponentis a styled-component, and if so, apply the.extendlogic under the hood?We're getting the deprecation warnings, but we can't actually upgrade until the
.extendfunctionality is restored under the hood ofstyled(), as was suggested in the original description of this issue.Current compatibility issues with
.withComponentare highlighted below:const Button = styled.button` // Imagine some styles here `; const ExtendedButton = Button.extend` color: green; `; const StyledButton = styled(Button)` color: green; `; // FAILS :( const StyledButtonLink = styled(StyledButton.withComponent('a'))` font-weight: bold; ` // PASSES :) (because we used .extend) const ExtendedButtonLink = styled(ExtendedButton.withComponent('a'))` font-weight: bold; `Here's a code pen demonstrating where this breaks down, and why we can't upgrade our app:
use as replace withComponent