Fix TypeScript support for custom components with non-standard declarations #2181
Conversation
🦋 Changeset detectedLatest commit: e05f3c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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. Latest deployment of this branch, based on commit e05f3c6:
|
|
Very interesting! I can confirm that it is fixing the problem in the linked codesandbox - although I don't understand what happens here either (gonna take another look later, with fresh mind). Could you try to add a type test for this? The PR that you have linked to has added some so you could take a look at how it has been done there. |
|
I know how to write a test, but I'm not exactly sure how to create a minimal reproducible example without bringing in RMWC because I don't know what specifically is causing the incompatibility. Perhaps further investigation could help, but I've already spent 3 days debugging this just to find this solution, and I don't really feel like doing any more. |
|
@Andarist I just added a test by effectively copy-pasting the RMWC definition and removing extraneous additions. As expected, it fails before this PR and works afterwards, but the issue still doesn't quite make sense. I suppose it doesn't really matter, since the patch makes logical sense and definitely works. |
|
Thank you! An interesting fact is that removing the intersection from our diff --git a/packages/react/types/jsx-namespace.d.ts b/packages/react/types/jsx-namespace.d.ts
index db09ddcf..1e45aca5 100644
--- a/packages/react/types/jsx-namespace.d.ts
+++ b/packages/react/types/jsx-namespace.d.ts
@@ -25,8 +25,9 @@ export namespace EmotionJSX {
extends ReactJSXElementAttributesProperty {}
interface ElementChildrenAttribute extends ReactJSXElementChildrenAttribute {}
- type LibraryManagedAttributes<C, P> = WithConditionalCSSProp<P> &
+ type LibraryManagedAttributes<C, P> = WithConditionalCSSProp<
ReactJSXLibraryManagedAttributes<C, P>
+ >
interface IntrinsicAttributes extends ReactJSXIntrinsicAttributes {}
interface IntrinsicClassAttributes<T>This is also fixing #2169 but, unfortunately, is causing some other test cases to fail for some reason. Nevertheless - your fix makes sense and even might come with some perf improvement so I'm going to merge this in and try to figure out the other thing later. |
What:
This PR makes a tiny modification to #2140 in order to fix support for components that do not use
React.ComponentTypebut rather a custom declaration with a generic render function.Why:
A certain package RMWC has been causing issues when using
"jsxImportSource": "@emotion/react". For instance, the types of the parameters passed to event handlers such asonClickwereany. I investigated the problem and discovered that the components were not declared withReact.ComponentType; instead, they had a generic render function to add support for atagproperty that changes the underlying DOM element used (e.g.tag="div"can be used to make a button use a div element internally). These changes resolve the issues and fix the typings on event handlers.How:
In all honesty: absolutely no idea. However, it seems to me that
LibraryManagedAttributesis meant to only specify the attributes that will be handled outside of React, whereas Emotion was previously including P in the intersection no matter what. This may have caused attributes to have been duplicated and merged with a union. For functions, unions make typing the parameters impossible in TypeScript, which would be the cause of the implicitanyissues. Of course, this is all speculation.Checklist:
Again, I'm really not sure how this works, but I do know that it does work.
CodeSandbox: https://codesandbox.io/s/gallant-snowflake-xt8dy