Fix mismatch between CSSInterpolation and Interpolation<Props> #3164
Conversation
Formerly, the first overload to be tried was not accepting template strings array as first argument. Therefore, it couldn't be used when `styled` was used as a tag for template string. So in this case TS skipped this overload and fell through to the next. Now, though, with `ArrayInterpolation` type changed, `TemplateStringsArray` matches the definition of `ArrayInterpolation`; therefore, this overload becomes used for template strings, confusing type inference. This change moves this overload to the end of the list, i.e. to be used as fallback when there's actually a direct function call, without template string.
🦋 Changeset detectedLatest commit: 2a76367 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
@emmatown do you happen to have time to review this PR? |
|
@Andarist do you happen to have time to review this PR? Sorry for repeated tagging, but I don't want this effort to get lost 😄 |
| @@ -52,7 +52,7 @@ export type Keyframes = { | |||
| } & string | |||
|
|
|||
| export interface ArrayInterpolation<Props> | |||
| extends Array<Interpolation<Props>> {} | |||
| extends ReadonlyArray<Interpolation<Props>> {} | |||
There was a problem hiding this comment.
this makes sense to me, it shouldnt hurt us anyhow
| @@ -63,10 +63,18 @@ export interface CreateStyledComponent< | |||
| SpecificComponentProps extends {} = {}, | |||
| JSXProps extends {} = {} | |||
| > { | |||
| ( | |||
| template: TemplateStringsArray, | |||
There was a problem hiding this comment.
I see where you are going with this reordering and I think it makes sense. However, I'm really anxious about reordering those overloads. This change needs some extra coverage in our type tests.
There was a problem hiding this comment.
Oh, I misunderstood this in part - this is actually a fix for existing tests (that became import after the change in @emotion/serialize)
|
@Cerber-Ursi @Andarist I believe this PR is related to this ticket 🤔 (haven't been able to verify): |
|
Thanks for notifying, will check soon. Seems that all this ReadonlyArray thing has quite a fallout actually... |
What:
Eliminated types mismatch between
CSSInterpolation(related tocsstypes) andInterpolation<Props>(related toemotion's own styled components machinery), which prevented the former from being used as the latter.Why:
This fixes #3145.
How:
This required two different changes:
ArrayInterpolation. Since it's expected that every interpolated value is treated byemotionas immutable, this change is correct, and it fixes the mismatch. At the same time, it breaks type inference on styled component creator functions, which required a second change.styled.Xfunctions. The idea is that we want to use "template string tag" overload wherever possible and fall back to another one only for the explicit function calls. Previously, these two overloads were independent and the order didn't matter, but now, as every validTemplateStringsArrayis also a validArrayInterpolation, we need to explicitly prioritize the "template string tag" case.Checklist: