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.

Sign up for GitHub

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

Jump to bottom

Let css prop attachment be distributed over union types. #3232

Merged
merged 6 commits into from
Aug 5, 2024
Merged

Let css prop attachment be distributed over union types. #3232

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a31e36c
Let 'css' prop attachment be distributed over union types.
ENvironmentSet Aug 1, 2024
1de1d20
Add a test case to check LibraryManagedAttributes distributes over un…
ENvironmentSet Aug 3, 2024
50eee88
Fix the test case
ENvironmentSet Aug 3, 2024
f2c7e55
Delete unused imports
ENvironmentSet Aug 4, 2024
01a2fd3
Replace with an integration test
ENvironmentSet Aug 5, 2024
f181c72
Update .changeset/heavy-pets-rule.md
Andarist Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/heavy-pets-rule.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@emotion/react': patch
---

Let 'css' prop attachment be distributed over union types.
Andarist marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 3 additions & 2 deletions packages/react/types/jsx-namespace.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ export namespace EmotionJSX {
extends ReactJSXElementAttributesProperty {}
interface ElementChildrenAttribute extends ReactJSXElementChildrenAttribute {}

type LibraryManagedAttributes<C, P> = WithConditionalCSSProp<P> &
ReactJSXLibraryManagedAttributes<C, P>
type LibraryManagedAttributes<C, P> = P extends unknown
? WithConditionalCSSProp<P> & ReactJSXLibraryManagedAttributes<C, P>
: never

interface IntrinsicAttributes extends ReactJSXIntrinsicAttributes {}
interface IntrinsicClassAttributes<T>
Expand Down
16 changes: 15 additions & 1 deletion packages/react/types/tests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import {
css,
jsx,
keyframes,
withEmotionCache
withEmotionCache,
Interpolation,
Theme
ENvironmentSet marked this conversation as resolved.
Show resolved Hide resolved
} from '@emotion/react'
import { JSX as EmotionJSX } from '@emotion/react/jsx-runtime'
import { CSSInterpolation } from '@emotion/serialize'
Expand Down Expand Up @@ -240,6 +242,18 @@ const anim1 = keyframes`
}
>['css']

/*
Additional brackets here prevent type reference('EmotionJSX.LibraryManagedAttributes<...>') from being resolved to type alias name('_HasCssPropAsIntended7').
Without them, dtslint will compare the expected type to the type name.
*/
type _HasCssPropAsIntended7 = [
// $ExpectType { foo: boolean; } | ({ css?: Interpolation<Theme>; } & { foo: number; className: string; })
EmotionJSX.LibraryManagedAttributes<
{},
{ foo: number; className: string } | { foo: boolean }
>
]
Copy link
Member

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" }}
/>;

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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!


// $ExpectType false
type _NoCssPropAsIntended1 =
'css' extends keyof EmotionJSX.LibraryManagedAttributes<
Expand Down
Loading