-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Let css
prop attachment be distributed over union types.
#3232
Let css
prop attachment be distributed over union types.
#3232
Conversation
🦋 Changeset detectedLatest commit: f181c72 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need some type-level tests to take in this change
Indeed, I'll add some. Would you please give me a guideline to add type level tests or example to refer to? I could not find either an example of type-level test or a setting relevant to it in emotion repository. |
You can add them here at the bottom of the file: https://github.com/emotion-js/emotion/blob/e819f9c39c4485997ce97fd2cfd330be8114e0f8/packages/react/types/tests.tsx |
Thanks, I added a test. Let me know if you need more. |
It seems dtslint compares type too strictly. I'll find other ways to express validity of the implementation. |
packages/react/types/tests.tsx
Outdated
type _HasCssPropAsIntended7 = [ | ||
// $ExpectType { foo: boolean; } | ({ css?: Interpolation<Theme>; } & { foo: number; className: string; }) | ||
EmotionJSX.LibraryManagedAttributes< | ||
{}, | ||
{ foo: number; className: string } | { foo: boolean } | ||
> | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is like a unit test. I am more interested in an integration test for this change. How did you come across this to be an issue? Maybe it was related to code like this?
type DiscriminatedClassNameSupportProp =
| {
variant: "foo";
className?: string;
}
| {
variant: "bar";
};
function DiscriminatedClassNameSupportComponent(
props: DiscriminatedClassNameSupportProp,
) {
return null;
}
<DiscriminatedClassNameSupportComponent
variant="foo"
// this should be OK
css={{ color: "hotpink" }}
/>;
<DiscriminatedClassNameSupportComponent
variant="bar"
// this shouldn't be OK
css={{ color: "hotpink" }}
/>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my case. May I add tests like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Please replace the tests you have here with something closer to the one above :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll update PR tonight. Thank you for comments!
What: Let
css
prop attachment be distributed over union types. So that branches withclassName
prop are extended withcss
prop.Why: Some components may accept
className
prop conditionally. For example,Link
of wouter does. Emotion skips these cases, however, and does not addcss
prop in anywhere. As far as I know, there is no runtime or compilation-time problem stopping us from supporting these kind of components. Thus I propose this change to enhance expressiveness of the library.fixes #3185
How: Made
LibraryManagedAttributes
be distributed over a prop type in cases it is a union.Checklist:
Should I add test cases for this change? Please let me know if you need.