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

Support JSX.ElementType #3048

Merged
merged 4 commits into from
Jun 7, 2023
Merged

Support JSX.ElementType #3048

merged 4 commits into from
Jun 7, 2023

Conversation

naari3
Copy link
Contributor

@naari3 naari3 commented Jun 4, 2023

What:

Support for JSX.ElementType that introduced in TypeScript 5.1

Why:

Until now, JSX type checking was provided by checking as JSX.Element.
From TypeScript 5.1 onward, by declaring JSX.ElementType, it will be possible to more flexibly check elements that can be handled on the jsx runtime side.

In React, this allows ReactNode to be used directly in the return value of function components. We would like to do the same for @emotion/react/jsx-runtime.

How:

In @types/react, JSX.ElementType has been declared since 18.2.8, and I have copied the definition from the 18.2.8. because I don't know how to handle the @types/react version in emotion.
If you don't mind updating @types/react, I can change it to use React.JSX.ElementType directly after updating!

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

closes #3049

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2023

🦋 Changeset detected

Latest commit: f3c4c64

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

codesandbox-ci bot commented Jun 4, 2023

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.

Latest deployment of this branch, based on commit f3c4c64:

Sandbox Source
Emotion Configuration
Emotion issue template Issue #3049
Emotion issue template Issue #3049

@@ -18,7 +18,12 @@ type ReactJSXIntrinsicAttributes = JSX.IntrinsicAttributes
type ReactJSXIntrinsicClassAttributes<T> = JSX.IntrinsicClassAttributes<T>
type ReactJSXIntrinsicElements = JSX.IntrinsicElements

// based on the code from @types/[email protected]
// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3197efc097d522c4bf02b94e1a0766d007d6cdeb/types/react/index.d.ts#LL3204C13-L3204C13
type ReactJSXElementType = string | React.JSXElementConstructor<any>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, likely we could do some hacks with @ts-ignore to "detect" if the type is there in React's JSX but perhaps it's not worth it and copy-paste is fine. I'll give this a thought.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I feel like copying it is probably the right approach tbh

@naari3
Copy link
Contributor Author

naari3 commented Jun 5, 2023

Hmm, why is this error happening here?

➤ YN0000: [@emotion/react]: Process started
➤ YN0000: [@emotion/react]: [email protected]
➤ YN0000: [@emotion/react]: Error: /home/runner/work/emotion/emotion/packages/react/types/tests.tsx:259:3
➤ YN0000: [@emotion/react]: ERROR: 259:3  expect  [email protected] compile error: 
➤ YN0000: [@emotion/react]: 'OtherComponent' cannot be used as a JSX component.
➤ YN0000: [@emotion/react]:   Its type '{ <Tag extends ElementType<any>>(props: AllHTMLAttributes<HTMLInputElement> | ComponentPropsWithRef<Tag>, ref: any): Element; displayName?: string | undefined; }' is not a valid JSX element type.
➤ YN0000: [@emotion/react]:     Type '{ <Tag extends ElementType<any>>(props: AllHTMLAttributes<HTMLInputElement> | ComponentPropsWithRef<Tag>, ref: any): Element; displayName?: string | undefined; }' is not assignable to type '(props: any) => ReactElement<any, any> | null'.
➤ YN0000: [@emotion/react]:       Target signature provides too few arguments. Expected 2 or more, but got 1.
➤ YN0000: [@emotion/react]: 
➤ YN0000: [@emotion/react]:     at testTypesVersion (/home/runner/work/emotion/emotion/node_modules/@definitelytyped/dtslint/dist/index.js:194:15)
➤ YN0000: [@emotion/react]:     at async runTests (/home/runner/work/emotion/emotion/node_modules/@definitelytyped/dtslint/dist/index.js:170:13)
➤ YN0000: [@emotion/react]:     at async main (/home/runner/work/emotion/emotion/node_modules/@definitelytyped/dtslint/dist/index.js:[79](https://github.com/emotion-js/emotion/actions/runs/5172904991/jobs/9317682961?pr=3048#step:4:80):9)
➤ YN0000: [@emotion/react]: Process exited (exit code 1), completed in 20s 272ms

@naari3
Copy link
Contributor Author

naari3 commented Jun 5, 2023

I don't know much about JS and TS, but it is hard to believe that this Pull Request change affects React.ElementType, so I thinking it was a @definitelytyped/dtslint issue and I updated to 0.0.115 and this error did not occur.
I don't know the details of why this problem no longer occurs with 0.0.115.

On the other hand, when I updated to the latest 0.0.163, I get the error everywhere. These seemed to be caused by existing type definitions that were now working fine.

@naari3
Copy link
Contributor Author

naari3 commented Jun 5, 2023

Sorry, I see that it has fallen unchanged in CI... On my local, the problem remains at 0.0.112 and remains resolved at 0.0.115.

Do you know anything about this?(why is this error occurring in the first place, Etc.)

@Andarist
Copy link
Member

Andarist commented Jun 5, 2023

  1. I think that this particular test case that is failing can be removed, this will revert~ Fix TypeScript support for custom components with non-standard declarations #2181 (cc @101arrowz) but that seems to be the correct thing to do. That PR was enabling something that isn't quite type-safe.
  2. so why introducing the JSX.ElementType here is creating a problem in the first place? With ElementType in place the tagType is being checked against elementTypeConstraint to see if it's assignable to it, whereas previously only return types were compared by TS using checkJsxReturnAssignableToAppropriateBound (see source code here)
  3. it happens so that we have @types/[email protected] installed here and that defines type JSXElementConstructor<P> = ((props: P) => React.ReactElement<any, any> | null) | (new (props: P) => React.Component<any, any>) but the current @types/[email protected] have a slightly different definition, see here.
  4. We can update @types/react as part of this PR (it has no effect on our consumers anyway, locally it's just a dev dependency and we forward whatever definitions are installed in consuming projects), which would fix the caught issue. The point is that how @types/react got adjusted 2 weeks ago is a temporary~ thing (see the PR here) so the test that is failing right now can stop working at just any time in the future
  5. we can double-check how those 2 JSXElementConstructor "perform" against this component in this TS playground. Why do I think that this definition isn't type-safe? This particular definition has a required second parameter in the signature of type any. This makes it somewhat OK since u forgo all the type safety with any anyway but if that would be unknown or just any other type then things could break badly. With JSX we are handing out functions to some runtime (in this case it's React) and that runtime is meant to call those functions on our behalf. But React can't call our functions with arbitrary extra arguments and yet our signatures might depend on them. In practice, this doesn't quite happen often and it's not a common problem but from the types PoV this is just wrong. It seems that TS has traditionally allowed this (see the playground here with the required string in the signature used as JSX element). The recent changes to JSX.ElementType will allow people to avoid such invalid combinations in the future (and already do it for cases with 3+ function parameters)

@naari3
Copy link
Contributor Author

naari3 commented Jun 5, 2023

@Andarist Wow, I am very happy to see a fairly thorough explanation! Thank you very much!

I tried each of the things you taught me, but this time I just decided to delete the test case.

Once I updated @types/react, this dragged the test cases in packages/native to fail. Probably due to the relationship between @types/react and @types/react-native, but I thought it was too broad to do in this PR.

If the test in the emotion fails, I thought about changing the ElementType as you said, but since that was not the case, I think we should leave it as is for the time being.

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@naari3
Copy link
Contributor Author

naari3 commented Jun 6, 2023

Thank you for your kind review, I'm so very happy! is there anything else I need to do to get this PR merged?

@Andarist Andarist merged commit 9357f33 into emotion-js:main Jun 7, 2023
@github-actions github-actions bot mentioned this pull request Jun 7, 2023
@karlhorky
Copy link

Thanks for the PR, review and merge @naari3, @Andarist, @emmatown ! 🙌

Looking forward to trying this out in @emotion/[email protected] after #3051 gets merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants