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

Deprecate .extend in favour of only styled(Component) #1546

Closed
pke opened this Issue Feb 22, 2018 · 68 comments

Comments

Projects
None yet
@pke

pke commented Feb 22, 2018

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?

@mxstbr

This comment has been minimized.

Member

mxstbr commented Feb 22, 2018

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)

@kitten

This comment has been minimized.

Member

kitten commented Feb 22, 2018

@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?

@mxstbr

This comment has been minimized.

Member

mxstbr commented Feb 22, 2018

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.

@kitten

This comment has been minimized.

Member

kitten commented Feb 22, 2018

@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

@mxstbr

This comment has been minimized.

Member

mxstbr commented Feb 22, 2018

I don't think folks will be particularly happy about us suddenly throwing warnings in a minor version upgrade.

@pke

This comment has been minimized.

pke commented Feb 22, 2018

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?

@mxstbr

This comment has been minimized.

Member

mxstbr commented Feb 22, 2018

Yeah exactly.

@kitten

This comment has been minimized.

Member

kitten commented Feb 22, 2018

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.

https://semver.org/

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.

@kitten kitten changed the title from Feature: Make styled() detect styled component and use .extend then to Deprecate .extend in favour of only styled(Component) Feb 22, 2018

@mxstbr

This comment has been minimized.

Member

mxstbr commented Feb 22, 2018

Yep, sounds very reasonable to batch that together with #1493! Maybe also #1171? I'd love to land that, personally.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Feb 28, 2018

We had a similar convo last year: #1087

I'm definitely in favor of getting rid of .extend from the public API.

@kitten

This comment has been minimized.

Member

kitten commented Feb 28, 2018

@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?

@mxstbr

This comment has been minimized.

Member

mxstbr commented Feb 28, 2018

Sounds perfect

@pke

This comment has been minimized.

pke commented Feb 28, 2018

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.

@kitten

This comment has been minimized.

Member

kitten commented Feb 28, 2018

@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 :)

@tysonmatanich

This comment has been minimized.

tysonmatanich commented Mar 3, 2018

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.

@denkristoffer

This comment has been minimized.

denkristoffer commented Mar 15, 2018

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'))
@mxstbr

This comment has been minimized.

Member

mxstbr commented Mar 15, 2018

@denkristoffer yes.

@pungggi

This comment has been minimized.

pungggi commented Mar 31, 2018

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'..

@ckeeney

This comment has been minimized.

ckeeney commented Apr 16, 2018

If extend is deprecated and removed, will there by an analogous .attrs() for styled?

@denkristoffer

This comment has been minimized.

denkristoffer commented Apr 16, 2018

@ckeeney attrs() works with styled() as it is:

const A = styled(B).attrs({
  style: ({ opacity }) => ({
    opacity,
  }),
})`
  color: red;
`
@ckeeney

This comment has been minimized.

ckeeney commented Apr 16, 2018

@probablyup probablyup added the 4.0 label Apr 17, 2018

@ppozniak

This comment has been minimized.

ppozniak commented Aug 28, 2018

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.

@RIP21

This comment has been minimized.

RIP21 commented Aug 28, 2018

@ppozniak looks absolutely valid.
Try to reproduce that in codesandbox.io and open issue if it's the case.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 28, 2018

I commented in the issue thread. The behavior is valid, just a misunderstanding of how the API works.

@greypants

This comment has been minimized.

greypants commented Aug 31, 2018

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:

https://codepen.io/greypants/pen/KxaBvx?editors=0010

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 31, 2018

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.

@greypants

This comment has been minimized.

greypants commented Aug 31, 2018

@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()?

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 31, 2018

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

ygkn added a commit to kinransenri-clubpc/YabaiScoreboard that referenced this issue Sep 9, 2018

@theboyWhoCriedWoolf

This comment has been minimized.

theboyWhoCriedWoolf commented Sep 10, 2018

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;
`;

@rainboxx rainboxx referenced this issue Sep 11, 2018

Merged

Add component-based global styling API #1867

3 of 3 tasks complete

zanedb added a commit to zanedb/hackathons that referenced this issue Sep 18, 2018

saghul added a commit to jitsi/jitsi-meet-electron that referenced this issue Sep 30, 2018

Fixed styled components warning
This is the new syntax for extending components. See:
styled-components/styled-components#1546
@mxstbr

This comment has been minimized.

Member

mxstbr commented Oct 1, 2018

@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 🎉

@RIP21

This comment has been minimized.

RIP21 commented Oct 1, 2018

@mxstbr glad to hear! If any issues let me know! :)

hristoterezov added a commit to jitsi/jitsi-meet-electron that referenced this issue Oct 4, 2018

Fixed styled components warning
This is the new syntax for extending components. See:
styled-components/styled-components#1546
@lauriejones

This comment has been minimized.

lauriejones commented Oct 24, 2018

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!

@RIP21

This comment has been minimized.

RIP21 commented Oct 24, 2018

@lauriejones

This comment has been minimized.

lauriejones commented Oct 24, 2018

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.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Oct 25, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment