Add stricter ReactInterpolation type #2297

Andarist · 2021-03-17T22:48:40Z

changeset-bot · 2021-03-17T22:48:42Z

⚠️ No Changeset found

Latest commit: 5e104f2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codesandbox-ci · 2021-03-17T22:49:55Z

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 5e104f2:

Sandbox	Source
Emotion	Configuration

emmatown · 2021-03-18T01:46:38Z

packages/react/types/index.d.ts

+type ReactArrayInterpolation<Props> = Array<ReactInterpolation<Props>>
+
+interface ReactFunctionInterpolation<Props> {
+  (props: Props): ReactInterpolation<Props>
+}
+
+export type ReactInterpolation<Props> =
+  | ReactInterpolationPrimitive
+  | ReactArrayInterpolation<Props>
+  | ReactFunctionInterpolation<Props>
+


Can we call the type parameter Theme since that's what it is here?

Ive also used it for one of the styled overloads where it is equivalent to Props

This type doesn't seem right for styled, e.g. styled.div('display:flex;') is valid.

Also, this change isn't right for the css prop either since css={['display:flex;']} is valid. The only thing that's wrong about the current types is that string isn't allowed at the root of the css prop.

Well, sure - the question is if we want to support this. Types don't always have to match the runtime reality fully. It feels better to guide users to better patterns if they are using a type checker.

@mitchellhamilton oh ok, I must have missed these style string patterns in the doc because I based my suggestion on what I saw demonstrated. If we really want to allow this form of string we can go ultra geeky and use template literal types but it seems overengineering at this point.

To be honest, the one thing I'm really interested in is for TS to catch me (and anyone starting with the css property) when I'm worngly assigning a class name to the css property.

So I'd agree with @Andarist here. It's about nudging and guiding to use some patterns rather that fully typing everything that could work but it's up to you folks.

Sure, let's change this for the css prop. Can you revert the change for @emotion/styled though since having @emotion/styled depend on a new thing from @emotion/react would be a breaking change since @emotion/styled has a peer dependency on @emotion/react? (also for styled, i believe we compile styles to styled.div('display: ', something, ';') and making it so our types treat something that we compile to as invalid seems bad)

Ok, I've removed this from @emotion/styled and also renamed Props to Theme since this will always be true right now.

This might be a user-facing change (some people might use Interpolation type in their code) but I think this is OK if we make this a minor change. Tying major versioning to types (unless changes are really extreme) falls outside of the pragmatic semver for me. What's your opinion on this?

jraoult · 2021-03-18T09:18:44Z

packages/react/types/index.d.ts

@@ -46,6 +46,24 @@ export function withEmotionCache<Props, RefType = any>(
  func: (props: Props, context: EmotionCache, ref: Ref<RefType>) => ReactNode
 ): FC<Props & ClassAttributes<RefType>>

+type ReactInterpolationPrimitive =


I think fundamentally what we mean is falsy or something that produce valid style information:

type ReactInterpolationPrimitive = | Falsy | SerializedStyles | CSSObject

Falsy in TS can be expressed like that (note the "" and false instead of boolean):

type Falsy = false | "" | 0 | null | undefined

Any other value would be probably the result of a programming mistake and would not be interpreted correctly by Emotion anyway, so we might as well catch them:

css={"my-custom-class"} css={true}

whereas the following could be the intended result of a boolean shortcircuit conditional:

css={""} css={false}

Hah, interesting that with someString && somethingElse TS actually computes the output as "" | typeof somethingElse! TIL

https://www.typescriptlang.org/play?#code/C4TwDgpgBAYghgGwM4igXigM0U6AfKAIkKgIAZSoA7AVwQUpqoBMJMBLKiZgKB9YDGCOACdoAgPZUkwLBIkAuKDJGcA5n0nTZcAQIhhg8ZKgwAKbCaXGUlAN5RJCCSKUr1UAL4BKdAD4oO09NKRkoMSR0OQkoADJYwMcJZ1dorx5dfUMbEDMI7xDtcIgkACYozHkoAH5EpxclSpjPKEb5DL0DIxxciNLvIA

@Andarist yes this is pretty neat. From the quick test of my code base (a branch where I'm refactoring to use the css property), these types already help me a lot in finding mistakes I made.

In your example, for res2, I expected TS to be a little smarter with the ternary operator. I can't really understand what other value foo can be in the right branch other then "" 🤷‍♂️..

So this case would generate a false negative, but to be fair I think this is a pattern that should be very rare when using Emotion. I can't really see what one would want to achieve with this where the shortcircuit conditional would be enough.

In your example, for res2, I expected TS to be a little smarter with the ternary operator. I can't really understand what other value foo can be in the right branch other then "" 🤷‍♂️..

Yep, same - both ternary and && should really behave the same way. This was for illustration purposes - not that relevant for this PR really (I don't feel that it's a show-stopper or anything).

FYI, after unsuccessfully trying to find the answer in TS repository, I just posted the question on SO

jraoult · 2021-03-27T18:58:02Z

@Andarist did you need help with this? I'm quite keen to see it merged so tell me if I can do anything.

Andarist · 2021-03-27T20:21:20Z

@jraoult adding type tests and checking what happens with failed CI jobs (the ones on the CircleCI) would certainly help

jraoult · 2021-03-30T15:26:42Z

@Andarist I took some time today to work on this. I think I'm ready to push. How should I proceed? Can you give me write access to this branch? Should I create another PR?

Andarist · 2021-03-30T16:48:47Z

U can create a PR targeting this branch

jraoult · 2021-03-30T17:41:47Z

@Andarist you can find the PR here: #2318

Andarist · 2021-04-10T18:39:03Z

@jraoult @mitchellhamilton passing string prop is actually supported in the codebase:

emotion/packages/react/src/emotion-element.js

Lines 66 to 74 in 4d7efcb

    
           // so that using `css` from `emotion` and passing the result to the css prop works 
        
           // not passing the registered cache to serializeStyles because it would 
        
           // make certain babel optimisations not possible 
        
           if ( 
        
             typeof cssProp === 'string' && 
        
             cache.registered[cssProp] !== undefined 
        
           ) { 
        
             cssProp = cache.registered[cssProp] 
        
           }

I guess this was mainly to make the Emotion 9 -> Emotion 10 easier but this code has not been removed in Emotion 11. This logic probably kicks in when providing the cache from @emotion/css to CacheProvider.

jraoult · 2021-04-10T19:01:18Z

passing string prop is actually supported in the codebase:

Exactly, this is actually what got me into this rabbit hole initially 😉 During refactoring, I noticed one could still give the css prop a class name generated by @emotion/css, and it would work. I then assumed from this finding that css would be able to take multiple class names in an array and that I could remove cx. It's only later that I realised it was a wrong assumption and that passing a string to css wasn't event documented.

So my initial proposal for this typing was to make sure no undocumented pattern could be used and help people avoid the pitfall.

Add stricter ReactInterpolation type

25c1e04

Andarist mentioned this pull request Mar 17, 2021

Types for react element's CSS property seem too permissive #2289

Open

emmatown reviewed Mar 18, 2021

View reviewed changes

jraoult reviewed Mar 18, 2021

View reviewed changes

jraoult mentioned this pull request Mar 30, 2021

Stricter react interpolation type [external changes] #2318

Open

Andarist added 2 commits April 8, 2021 13:22

Revert using ReactInterpolation in @emotion/styled

0c02ae3

Rename Props generic to Theme in ReactInterpolation-related types

5e104f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stricter ReactInterpolation type #2297

Add stricter ReactInterpolation type #2297

Andarist commented Mar 17, 2021

changeset-bot bot commented Mar 17, 2021 •

edited

Loading

codesandbox-ci bot commented Mar 17, 2021 •

edited

Loading

emmatown Mar 18, 2021

Andarist Mar 18, 2021

emmatown Mar 18, 2021

Andarist Mar 19, 2021

jraoult Mar 19, 2021

emmatown Mar 22, 2021

Andarist Apr 8, 2021

jraoult Mar 18, 2021 •

edited

Loading

Andarist Mar 18, 2021

jraoult Mar 18, 2021

Andarist Mar 18, 2021

jraoult Mar 18, 2021 •

edited

Loading

jraoult commented Mar 27, 2021

Andarist commented Mar 27, 2021

jraoult commented Mar 30, 2021

Andarist commented Mar 30, 2021

jraoult commented Mar 30, 2021

Andarist commented Apr 10, 2021

jraoult commented Apr 10, 2021

Add stricter ReactInterpolation type #2297

Are you sure you want to change the base?

Add stricter ReactInterpolation type #2297

Conversation

Andarist commented Mar 17, 2021

changeset-bot bot commented Mar 17, 2021 • edited Loading

⚠️ No Changeset found

codesandbox-ci bot commented Mar 17, 2021 • 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

Choose a reason for hiding this comment

jraoult Mar 18, 2021 • 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

jraoult Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

jraoult commented Mar 27, 2021

Andarist commented Mar 27, 2021

jraoult commented Mar 30, 2021

Andarist commented Mar 30, 2021

jraoult commented Mar 30, 2021

Andarist commented Apr 10, 2021

jraoult commented Apr 10, 2021

changeset-bot bot commented Mar 17, 2021 •

edited

Loading

codesandbox-ci bot commented Mar 17, 2021 •

edited

Loading

jraoult Mar 18, 2021 •

edited

Loading

jraoult Mar 18, 2021 •

edited

Loading