Fix `css` prop missing from props types in TypeScript #2140

hasparus · 2020-11-27T19:15:42Z

What:

Sup folks. I fixed one small problem with WithConditionalCSSProp.

I was actually fixing a similar thing in another library system-ui/theme-ui#1307.
I accidentaly auto-imported WithConditionalCSSProp instead of WithConditionalSxProps while writing tests, and I noticed you have exactly the same problem :D

Why:

css prop is not added to all props types which accept className of type string, and it's also added to props which accept className?: undefined.

Take a look at this TS Playground.

How:

We can read extends as is assignable to or is subset of.
We need to add css prop to all props objects that can accept className prop of type string, so

if { className: string } is assignable to P
or if we think of sets, if a set of all objects that have className property of type string is a subset of set P.

Checklist:

Documentation N/A
Tests
Code complete
Changeset

changeset-bot · 2020-11-27T19:15:44Z

🦋 Changeset detected

Latest commit: 2c7c2b9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

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 · 2020-11-27T19:33:08Z

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 2c7c2b9:

Sandbox	Source
Emotion	Configuration

hasparus · 2020-11-27T19:43:03Z

I see I failed tests on CircleCi. I'll take a look at it ~~in a moment~~ tomorrow :)

Oh wow, I know what I missed :o

Andarist · 2020-11-28T15:16:29Z

packages/react/types/jsx-namespace.d.ts

@@ -2,8 +2,12 @@ import 'react'
 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)
+// prettier-ignore


Any reason why this is ignored here? Its in the source code so formatting shouldnt affect much

Sorry, this isn't necessary anymore. Thanks!

Andarist · 2020-11-28T15:17:26Z

packages/react/types/tests.tsx


  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' }} />


Why this has to be changed? 🤔

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.

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

Andarist · 2020-11-28T15:20:41Z

packages/react/types/tests.tsx

+}
+
+// Tests for WithConditionalCSSProp
+{


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

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.

Andarist · 2020-11-28T15:23:45Z

packages/react/types/jsx-namespace.d.ts

+// prettier-ignore
+type WithConditionalCSSProp<P> =
+  'className' extends keyof P
+    ? string extends P['className' & keyof P]


Could u explain the machinery if this type step by step? Especially the & keyof bit - is this just supposed to produce never if there is no className prop?

Ill have to think the whole thing through in terms of possible perf regressions as well.

sure. I assume you get the gists of it, so I'll focus on that confusing & keyof.

Let's just translate it into pseudocode, so we're on the same page.

if className is a key in Props then if Props["className"] can be a string then return Props merged with { css?: Interpolation<Theme> } else return Props else return Props

Previous code (pasted below for comparison) was checking can I assign Props to { className?: string }.
You can assign { className?: undefined } to { className?: string }, and you can't assign { className: string[] | string } or { className: unknown } to it.

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

Especially the & keyof bit - is this just supposed to produce never if there is no className prop?

Yup, that's correct.

This is the most "interesting" part. The props will surely be an object, and we just care about the className prop.
But we can't read that className from P until we know that such a key exists in P.

This code is sufficient in TS 4.0, but TS 3.6 is a bit dumber.
I had to add that & keyof P, because className is really a key of P at this point, and we'll never get never there. TS 3.6 just doesn't remember that we already checked it :)

But hey! Doesn't this mean that we can get rid of that first extends keyof P clause and use only one conditional type?

Like so

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

We could if we didn't care about TS 3.4 anymore.

3.4 is before TypeScript was smart enough to know that you can't be two different strings at once. An intersection of 'any-string' & 'className' doesn't collapse into never, we get an error, and a nice any, css is added to all props regardless if they have className and our tests fail.

That dtslint was a smart choice.

Ill have to think the whole thing through in terms of possible perf regressions as well.

Do you have any benchmarks / performance tests for Emotion types?

I have done Very Scientific Research™ by checking a fresh CRA with just a few css prop-based components and those are some numbers from the --diagnostics (I've ignored mem consumption and times as those are not stable between reruns):

Current

Nodes: 336996 Identifiers: 114711 Symbols: 81060 Types: 4642 Instantiations: 14645

This PR

Nodes: 336998 Identifiers: 114711 Symbols: 80811 Types: 4181 Instantiations: 4824

Without & keyof P

Nodes: 336994 Identifiers: 114710 Symbols: 80811 Types: 4180 Instantiations: 4809

As we may notice there is only a rather negligible difference between your PR and the "without & keyof P" variant that aims to satisfy older TS versions but there is actually a big improvement over the current version in terms of instantiations that are often responsible for slowdowns

.changeset/perfect-spoons-whisper.md

hasparus added 2 commits November 27, 2020 19:54

Invert extends in WithConditionalCSSProp

7200754

Add tests to WithConditionalCSSProp type

ceeb227

Update jsx-namespace.d.ts

b395757

hasparus added 2 commits November 27, 2020 22:05

Use Public API in test

8d44d47

Remember that components can have more than one prop 🤦

234f6a8

hasparus force-pushed the inverted-css-prop branch from a9f2994 to 234f6a8 Compare November 27, 2020 22:57

Work around Prettier betrayal

f21230f

Andarist reviewed Nov 28, 2020

View reviewed changes

hasparus added 3 commits November 29, 2020 21:19

Add changeset

1913114

Clean up prettier-ignore

8b9e611

Add dtslint $ExpectType comments

652af83

hasparus requested a review from Andarist November 29, 2020 21:17

Andarist reviewed Dec 1, 2020

View reviewed changes

.changeset/perfect-spoons-whisper.md Outdated Show resolved Hide resolved

Update .changeset/perfect-spoons-whisper.md

2c7c2b9

Andarist merged commit e584353 into emotion-js:master Dec 1, 2020

github-actions bot mentioned this pull request Dec 1, 2020

Version Packages #2128

Merged

101arrowz mentioned this pull request Dec 20, 2020

Fix TypeScript support for custom components with non-standard declarations #2181

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `css` prop missing from props types in TypeScript #2140

Fix `css` prop missing from props types in TypeScript #2140

hasparus commented Nov 27, 2020 •

edited

Loading

changeset-bot bot commented Nov 27, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 27, 2020 •

edited

Loading

hasparus commented Nov 27, 2020 •

edited

Loading

Andarist Nov 28, 2020

hasparus Nov 29, 2020

Andarist Nov 28, 2020

hasparus Nov 29, 2020

Andarist Nov 29, 2020

Andarist Nov 28, 2020

hasparus Nov 29, 2020 •

edited

Loading

Andarist Nov 28, 2020

hasparus Nov 29, 2020

Andarist Dec 1, 2020

Fix css prop missing from props types in TypeScript #2140

Fix css prop missing from props types in TypeScript #2140

Conversation

hasparus commented Nov 27, 2020 • edited Loading

changeset-bot bot commented Nov 27, 2020 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Nov 27, 2020 • edited Loading

hasparus commented Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasparus Nov 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Current

This PR

Without & keyof P

Fix `css` prop missing from props types in TypeScript #2140

Fix `css` prop missing from props types in TypeScript #2140

hasparus commented Nov 27, 2020 •

edited

Loading

changeset-bot bot commented Nov 27, 2020 •

edited

Loading

codesandbox-ci bot commented Nov 27, 2020 •

edited

Loading

hasparus commented Nov 27, 2020 •

edited

Loading

hasparus Nov 29, 2020 •

edited

Loading

Without `& keyof P`