styled-components / styled-components

Visual primitives for the component age. Use the best bits of ES6 and CSS to style your apps without stress 💅
https://styled-components.com
MIT License
40.11k stars 2.48k forks source link

fix: improve types for .attrs() #4288

Closed uhyo closed 1 week ago

uhyo commented 1 month ago

closes #4076 closes #4138 closes #4183

Hello, the above issue is blocking us from upgrading to v6, so here is a possible fix.

Improvement

The improvement brought by this PR can be seen in this example:

  const DivWithRequiredFooBar = styled.div<{ foo: number; bar: string }>``;

  // foo is provided, so it becomes optional
  const DivWithRequiredBar = styled(DivWithRequiredFooBar).attrs({ foo: 42 })`
    margin; ${props => props.foo * 10}px;
  `;
  // @ts-expect-error must provide bar
  <DivWithRequiredBar />;
  // OK (NEW)
  <DivWithRequiredBar bar="3" />;
  // OK. Can still provide foo if we want
  <DivWithRequiredBar foo={3} bar="3" />;
  // @ts-expect-error foo must be a number
  <DivWithRequiredBar foo="3" bar="3" />;

The part marked as (NEW) is made possible by this PR. Once foo is provided through the .attrs call, the resulting component (DivWithRequiredBar) no longer requires foo as a prop.

Implementation

To achieve this, the Styled interface now has distinction between Outer props and Inner props:

  const DivWithRequiredBar = styled(DivWithRequiredFooBar).attrs({ foo: 42 })`
    // ↓ `props` here is the inner props which have undergone the defaulting,
    // so `props.foo` is always supplied (not optional)
    margin; ${props => props.foo * 10}px;
  `;
  // User of the resulting component sees the outer props,
  // so foo is optional here
  <DivWithRequiredBar bar="3" />;

Thus the Styled interface now has new InnerProps type attribute. It is usually same as OuterProps, but the updated attrs() types can make them diverge.

AntonNiklasson commented 1 week ago

@quantizor I'm trying to run 6.1.9, but it seems like that version contains an older version of the code. Was this change actually included in the published code?

AntonNiklasson commented 1 week ago

Perhaps I'm missing something. It doesn't do what it's supposed to do though 🤷‍♂️

uhyo commented 1 week ago

@AntonNiklasson I checked and confirmed that 6.1.9 does what is written this PR.

It's possible that I'm missing something though, so I recommend opening a new issue with concrete examples. I think I can take a look then, Unfortunately your 🤷‍♂️ doesn't carry enough information for now.

AntonNiklasson commented 6 days ago

I'm sorry for the rude tone in my previous comment @uhyo. That didn't come out the way I wanted it to. I really appreciate all the hard work you've put into this!

I've been trying to reproduce it, I'll open a new issue if I find something. What I'm seeing right now is just that the props I explicitly set in .attrs() isn't affecting the type in the final component.

uhyo commented 6 days ago

No problem! Anyways I see that several new issues are already out there regarding the 6.1.9 release. I'm so sorry for this additional maintenance burden. I'm going to check all of them.

AntonNiklasson commented 4 days ago

@quantizor I saw you reverted the changes here. Is there another initiative to solve the typing for .attrs? It would be so nice to get it fixed, we’re stuck on v5 still.

quantizor commented 4 days ago

Will revisit later as a test release. I'm not keen to keep putting out problematic typing mainline releases...

AntonNiklasson commented 4 days ago

That makes sense! Perhaps it would be worth it to invest a bit into a types testing setup? I noticed there’s nothing in place when I played with #4306.

quantizor commented 4 days ago

There are a few type test files in the repo

quantizor commented 4 days ago

The issue is more about scale. This repo might be fine but a large one with hundreds or thousands of files will have entirely different compile performance...