Fix css prop missing from props types in TypeScript

[hasparus]

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]

🦋 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]

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]

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]

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

[hasparus]

Sorry, this isn't necessary anymore. Thanks!

[Andarist]

Why this has to be changed? 🤔

[hasparus]

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.

[Andarist]

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

[Andarist]

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

[hasparus]

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]

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.

[hasparus]

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?

[Andarist]

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