Skip to content

Conversation

@emmatown
Copy link
Member

This regressed in #3232

Fixes #3245
Fixes #3261

@emmatown emmatown requested a review from Andarist December 12, 2024 01:19
@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2024

🦋 Changeset detected

Latest commit: 8aacec3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@emotion/react Patch

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

@codesandbox-ci
Copy link

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 emmatown marked this pull request as draft December 12, 2024 03:12
@emmatown
Copy link
Member Author

emmatown commented Dec 12, 2024

Hmmm, actually this solution is wrong (in that it always adds css prop) and makes the types more complex for the compiler (the props of a component with a className prop being (TheActualProps & {}) | (TheActualProps & { css?: Interpolation })). I think the right solution is probably just not being distributive and knowingly not supporting the use case of #3185. Being able to write a higher order component at all without type errors is more valuable than a component conditionally supporting className having the css prop accessible in types.

@emmatown emmatown closed this Dec 12, 2024
@Andarist
Copy link
Member

Being able to write a higher order component at all without type errors is more valuable than a component conditionally supporting className having the css prop accessible in types.

I agree. It seems that TS isn't able to relate a non-conditional type to a distributive conditional type (this is the only branch that checks targetFlags & TypeFlags.Conditional outside of sourceFlags & TypeFlags.Conditional):
https://github.dev/microsoft/TypeScript/blob/6a00bd2422ffa46c13ac8ff81d7b4b1157e60ba7/src/compiler/checker.ts#L23200-L23221

It doesn't even work in cases like this (TS playground):

declare function test<T>(
  p1: T,
  p2: T extends unknown ? T & { css?: unknown } : never,
): void;

const wrapper = <P__ extends object>(props: P__) => {
  test(
    props,
    props // error
  );
};

It seems that perhaps this rule is overly restrictive but I don't know how to safely loosen it to handle this case. And any potential fix wouldn't land in TS soon enough for us anyway - so ye, we have to revert that PR.

@Andarist
Copy link
Member

Andarist commented Dec 19, 2024

I opened a PR that could fix this but I can't really say with confidence that it's a correct one: microsoft/TypeScript#60817

EDIT:// spoiler alert: it wasn't correct

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v11.13.3 type error with Next Props not assignable to LibraryManagedAttributes type error

3 participants