Skip to content
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

Fix TypeScript error when writing higher order components while using Emotion's JSX namespace #3293

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

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.

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