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

Fix css prop missing from props types in TypeScript #2140

Merged
merged 10 commits into from
Dec 1, 2020
Merged

Fix css prop missing from props types in TypeScript #2140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7200754
Invert `extends` in WithConditionalCSSProp
hasparus Nov 27, 2020
ceeb227
Add tests to WithConditionalCSSProp type
hasparus Nov 27, 2020
b395757
Update jsx-namespace.d.ts
hasparus Nov 27, 2020
8d44d47
Use Public API in test
hasparus Nov 27, 2020
234f6a8
Remember that components can have more than one prop 🤦
hasparus Nov 27, 2020
f21230f
Work around Prettier betrayal
hasparus Nov 27, 2020
1913114
Add changeset
hasparus Nov 29, 2020
8b9e611
Clean up prettier-ignore
hasparus Nov 29, 2020
652af83
Add dtslint $ExpectType comments
hasparus Nov 29, 2020
2c7c2b9
Update .changeset/perfect-spoons-whisper.md
Andarist Dec 1, 2020
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/perfect-spoons-whisper.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@emotion/react': patch
---

Fixed an issue with `css` prop type not being added to all components that accept a string `className` correctly.
4 changes: 3 additions & 1 deletion packages/react/types/jsx-namespace.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { Interpolation } from '@emotion/serialize'
import { Theme } from './index'

type WithConditionalCSSProp<P> = 'className' extends keyof P
? (P extends { className?: string } ? P & { css?: Interpolation<Theme> } : P)
? string extends P['className' & keyof P]
? P & { css?: Interpolation<Theme> }
: P
: P

// unpack all here to avoid infinite self-referencing when defining our own JSX namespace
Expand Down
55 changes: 51 additions & 4 deletions packages/react/types/tests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
keyframes,
withEmotionCache
} from '@emotion/react'
import { JSX as EmotionJSX } from '@emotion/react/jsx-runtime'

declare module '@emotion/react' {
// tslint:disable-next-line: strict-export-declare-modifiers
Expand Down Expand Up @@ -159,14 +160,14 @@ const anim1 = keyframes`

// TS@next reports an error on a different line, so this has to be in a single line so `test:typescript` can validate this on all TS versions correctly
// $ExpectError
;<CompWithoutClassNameSupport prop1="test" css={{ backgroundColor: 'hotpink' }} />
;<CompWithoutClassNameSupport prop1="test" css={{ color: 'hotpink' }} />

const MemoedCompWithoutClassNameSupport = React.memo(
CompWithoutClassNameSupport
)
// TS@next reports an error on a different line, so this has to be in a single line so `test:typescript` can validate this on all TS versions correctly
// $ExpectError
;<MemoedCompWithoutClassNameSupport prop1="test" css={{ backgroundColor: 'hotpink' }} />
;<MemoedCompWithoutClassNameSupport prop1="test" css={{ color: 'hotpink' }} />
Copy link
Member

Choose a reason for hiding this comment

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

Why this has to be changed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line with backgroundColor is too long for Prettier. Even if you don't format it on auto-save in your IDE, you need to remember to do git commit --no-verify, because there's a prettier --write running on pre-commit.

  "lint-staged": {
    "*.{js,ts,tsx,md}": [
      "prettier --write",
      "git add"
    ]
  }

Obviously, I kept forgetting about it and failing tests, because the error occurs in different places (on css or over entire element) by different TypeScript version.

I can change it to backgroundColor if you want, but that property is irrelevant from the perspective of the test, because the css prop is missing, and with backgroundColor it was just more unpleasant to contribute to this file.

Copy link
Member

Choose a reason for hiding this comment

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

Right - I don't mind the change, was just curious why it was needed. Your explanation makes sense 👍

}

{
Expand All @@ -181,6 +182,52 @@ const anim1 = keyframes`
// https://github.com/DefinitelyTyped/DefinitelyTyped/issues/40993
// this is really problematic behaviour by @types/react IMO
// but it's what @types/react does so let's not break it.
const CompWithImplicitChildren: React.FC = () => null;
;<CompWithImplicitChildren>content<div/></CompWithImplicitChildren>
const CompWithImplicitChildren: React.FC = () => null
;<CompWithImplicitChildren>
content
<div />
</CompWithImplicitChildren>
}

// Tests for WithConditionalCSSProp
{
Copy link
Member

Choose a reason for hiding this comment

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

Im reviewing from mobile and everything is squeezed so maybe im seeing this wrong but it seems that those tests only compute types. Couldwe rewrite them in a way that they would act as assertions? So tests would a tually fail if we introduce a regression? Such test could be written by comparing the output to the expected type

Copy link
Contributor Author

@hasparus hasparus Nov 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. I wrote hoping they'll crash if there's a regression, but any any in LibraryManageAttributes could wreak them. I can certainly improve them with $ExpectType.

// $ExpectType Interpolation<Theme>
type _HasCssPropAsIntended3 = EmotionJSX.LibraryManagedAttributes<
{},
{
className?: string
}
>['css']

// $ExpectType Interpolation<Theme>
type _HasCssPropAsIntended4 = EmotionJSX.LibraryManagedAttributes<
{},
{
className: string
}
>['css']

// $ExpectType Interpolation<Theme>
type _HasCssPropAsIntended5 = EmotionJSX.LibraryManagedAttributes<
{},
{
className?: unknown
}
>['css']

// $ExpectType Interpolation<Theme>
type _HasCssPropAsIntended6 = EmotionJSX.LibraryManagedAttributes<
{},
{
className?: string | Array<string>
}
>['css']

// $ExpectType false
type _NoCssPropAsIntended1 = 'css' extends keyof EmotionJSX.LibraryManagedAttributes<
{},
{ className?: undefined }
>
? true
: false
}