chore(styled): let StyledOptions generic argument be optional #2333

antongolub · 2021-04-06T08:38:51Z

We're now migrating from v10 and noticed a strange thing. StyledOptions now requires Props argument, which does not affect at all. What do you think about either removing the param or at least making it optional?

declare type PropertyKey = string | number | symbol;
...
export interface StyledOptions<Props> {
  label?: string
  shouldForwardProp?(propName: PropertyKey): boolean
  target?: string
}

changeset-bot · 2021-04-06T08:38:53Z

🦋 Changeset detected

Latest commit: 682313b

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

This PR includes changesets to release 3 packages

Name	Type
@emotion/native	Patch
@emotion/styled	Patch
@emotion/is-prop-valid	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 · 2021-04-06T08:44:22Z

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 682313b:

Sandbox	Source
Emotion	Configuration

Andarist · 2021-04-06T10:47:11Z

Please prepare a runnable repro case of your problem or add a type test (in the types directory) that would showcase why this is needed.

antongolub · 2021-04-06T14:16:07Z

Hey, @Andarist,

This is not a problem. Once we've bumped emotion version in our project, we just found API diff, which does not add any functionality.
#1643 brought this relaxed type earlier, so now generic seems useless here.

Andarist · 2021-04-06T17:33:34Z

Hm, it should still be possible to add a test type for this - since some code did not work for you and you expect it to work.

antongolub · 2021-04-06T17:39:32Z

Sounds reasonable. I'll do.

Andarist · 2021-04-07T16:28:04Z

packages/native/types/base.d.ts

@@ -64,7 +64,7 @@ export interface FilteringStyledOptions<
  shouldForwardProp?(propName: PropertyKey): propName is ForwardedProps
 }

-export interface StyledOptions<Props> {
+export interface StyledOptions<Props = Record<PropertyKey, any>> {


well, this generic stays unused so I think it should be more than OK to just drop it

Yep. I suggest using this impl cause it's compatible with both v10 and v11-based app code. Removing generic will be a breaking change for someone. Is it acceptable?

Any change is a breaking change for someone 😅 I think in this case it's a pragmatic choice to not treat it as one - especially give that I don't expect StyledOptions to be used explicitly in a lot of codebases.

Maybe it's better to use generic argument if passed?

export interface StyledOptions<Props = null> { label?: string shouldForwardProp?(propName: Props extends null ? PropertyKey : keyof Props): boolean target?: string }

No breaking change

Compatible with v10 and v11 codebases

Useful type check

Quite honestly, I'm not sure if it's worth it. This type should not be used that much publicly and we'd have to check if this would be compatible when passed to styled calls. It would also be good to dig up the PRs by @JakeGinnivan which were related to this and make sure that this would not break because of the reasons mentioned there. Ideally, we would pass Props to StyledOptions from within CreateStyledComponent types.

If you are willing to do the work - then sure, ye, we could try to do that.

Well, if this is supposed to land then I strongly believe that those 2 cases should not result in any type errors - even if the problem is not related to this PR it IMHO would be preferable to fix it here or prepare a separate fix, land it and then rebase this PR. Without those working it's hard for me to justify merging this PR with that generic being used.

Give me a bit more time. I'll fix the issue above.

No problem - there is no rush. I appreciate your efforts here :)

@Andarist,

Not sure if I found the best solution, but it seems working.

@Andarist,
Could you take a look?

…nature fixes emotion-js#2333 (comment)

packages/native/types/base.d.ts

packages/styled/types/tests.tsx

packages/styled/types/base.d.ts

packages/styled/types/tests.tsx

antongolub · 2021-04-26T12:01:02Z

@Andarist, any suggestions/objections?

antongolub · 2021-05-10T06:22:39Z

@Andarist, let’s try to move this forward.

Andarist · 2021-05-10T06:28:47Z

Sorry for the delay but I was lately very busy. Please be patient - I'll get to reviewing this PR when I get some time for it.

antongolub · 2021-06-11T15:57:36Z

@Andarist, @mitchellhamilton,

Do you need any help with this? Maybe I should change or add something to make it easier for review? We are really looking forward to this PR change.

Andarist · 2022-02-25T16:59:12Z

packages/styled/types/base.d.ts

 export interface FilteringStyledOptions<
-  Props,
+  Props = Record<PropertyKey, any>,


q: why the Props have a different default between FilteringStyledOptions and StyledOptions?

@Andarist

Props = Record<PropertyKey, any>,

This is just an artifact of the first impl. I'll fix.

So it seems like the only thing to settle on is the variance type for the shouldForwardProp.

It seems that strictFunctionTypes option is not applied to object method?() {...} signature. So if we rewrite the code as suggested above (method?: () => {...}) we break everything.

export interface FilteringStyledOptions< Props = AnyRecord, ForwardedProps extends keyof Props = keyof Props > { label?: string shouldForwardProp?: <P extends UnionToIntersection<ForwardedProps>>(propName: P) => propName is P target?: string } export interface StyledOptions<Props = AnyRecord> { label?: string shouldForwardProp?: <P extends keyof UnionToIntersection<Props>>(propName: P) => boolean target?: string } type AnyRecord = Record<PropertyKey, any> export type UnionToIntersection<U, K = any> = ( U extends K ? (k: U) => void : never ) extends (k: infer I) => void ? I : never

The main problem: I don't know how to infer param type inside generic, so I cannot add a type assertion that impl fn param extends HTML element attributes.

type F = <P extends any extends infer P ? P : never>(p: P) => boolean // P is unknown const f: F = (prop: 'style') => true

I also tried to handle this in another way: to represent shouldForwardProp as a union of fns with all possible signatures. I looks syntactically correct and automatable with recursive mapping, but it requires a quantum computer.

type T = 'a' | 'b' | 'c' type F = (p: 'a') => true | (p: 'b') => true | (p: 'c') => true | (p: 'a' | 'b') => true | (p: 'b' | 'c') => true | (p: 'a' | 'c') => true | (p: 'a' | 'b' | 'c') =>

For ~300 attributed for every HTML element type we get: C(n,k) = n! / k!(n-k)!, k = 1, n=300, C =~3E615.
https://www.numberempire.com/factorialcalculator.php?number=300

PS Sorry for the late reply.

It seems that strictFunctionTypes option is not applied to object method?: () {} signature

If I understand this correctly then with strictFunctionTypes: false every function is bivariant but with strictFunctionTypes: true methods are still bivariant but every other function becomes contravariant.

I think you might have a typo in this example as it should be method?(): void (or similar) to fit the rest of the sentence. I've just wanted to clarify that we have a common understanding of this stuff (where I'm not 100% sure if my understanding is fully correct).

So if we rewrite the code as suggested above we break everything.

Does "as suggested" mean "change to a function type and thus make it contravariant"? Could you give me an example of what this would break? If it breaks something important then I think we should have a type test for this.

shouldForwardProp?: <P extends UnionToIntersection>(propName: P) => propName is P

I'm unsure what's happening here. I understand what UnionToIntersection does but I don't see why it was applied here. The result of this type is an intersection and P definitely should be constrained to a union of strings (or to just a string type). Could you explain what was the goal here?

I've pushed several snapshot branches so you can observe my poor efforts:

snap#3 All gneric singatures use Record<PropertyKey, any> as default type. Filters types exposed as object fields. The only working.

snap#2 shouldForwardProp type declared as a generic function + UnionToIntersection

snap#1 same as 2 but w/o UnionToIntersection.

UnionToIntersection was placed here to ensure that keyof type includes all the keys of all types of Props union:
ClassAttributes<HTMLDivElement> | HTMLAttributes<HTMLDivElement> → keyof ClassAttributes<HTMLDivElement> | keyof HTMLAttributes<HTMLDivElement>. Just as experiment.

It seems that we ran into some limitation (in understanding?) of the current TS. What I want to get:

// Declare filter signature that operates the keys of set 'a' | 'b' ... 'z' type F { filter: (p: 'a' | 'b' ... 'z') => bool } // Introduce a specific filter, that uses a subset p type const f: F = { filter: (p: 'a' | 'c') => bool } // 'a' | 'c' extends 'a' | 'c' ... 'z'

btw, tsconfig changes were required by dtslint:

% dtslint types Error: Expected `"strictFunctionTypes": true` or `"strictFunctionTypes": false`. at Object.<anonymous> (/Users/a.golub/projects/gh/emotion/node_modules/dtslint/bin/checks.js:95:23) at Generator.next (<anonymous>) at fulfilled (/Users/a.golub/projects/gh/emotion/node_modules/dtslint/bin/checks.js:4:58)

I've pushed several snapshot branches so you can observe my poor efforts:

Thank you for those - I'll try to review them this week to get a better feeling of what we are dealing with here.

UnionToIntersection was placed here to ensure that keyof type includes all the keys of all types of Props union:

Do we need to care about ClassAttributes though? This interface only contains key and ref properties and those can't really b received by shouldForwardProp as those are special props in React and it won't ever handle them to us.

It seems that we ran into some limitation (in understanding?) of the current TS. What I want to get:

Ye, this is impossible - mainly because now you could make such a call f.filter('z') and it could potentially crash at runtime as the supplied implementation is not capable of handling 'z'.

So basically, here, assigning to this type is not valid. Such things could be expressed with some function helpers and stuff, like here:
TS playground

The gist of this is that we preserve the type of the implementation function but we check if it's valid according to our rules. However, I'm not sure when this would be needed/helpful in our case - probably a full example involving those filters and a styled call would help me to see this (maybe it's already in those branches of yours, I didn't yet have a chance to look there though)

Andarist · 2022-02-25T16:59:30Z

So it seems like the only thing to settle on is the variance type for the shouldForwardProp. Let me know what do you think about this.

👋

Andarist · 2022-03-05T13:11:23Z

FYI just 2 small questions to be answered here and we should be able to land this soon. No rush, of course 😅

…ilteringStyledOptions

packages/native/types/tsconfig.json

packages/styled/types/tsconfig.json

srmagura · 2022-05-21T20:19:23Z

packages/styled/types/base.d.ts

-/** Same as StyledOptions but shouldForwardProp must be a type guard */
+/**
+ * Same as StyledOptions but shouldForwardProp must be a type guard (https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates).
+ * Practical sense: you can define and reuse atomic `shouldForwardProp` filters that are strictly bound with some `ForwardedProps` type.


I'm not sure what this line is saying... maybe rewording it or including an example would be helpful.

srmagura · 2022-05-21T20:20:21Z

packages/styled/types/base.d.ts

@@ -13,19 +17,22 @@ export {

 export { ComponentSelector, Interpolation }

-/** Same as StyledOptions but shouldForwardProp must be a type guard */
+/**
+ * Same as StyledOptions but shouldForwardProp must be a type guard (https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates).


I don't think this link to the TS handbook is necessary, and it will eventually become outdated if they release a new version of the handbook.

srmagura · 2022-05-21T20:26:19Z

packages/styled/types/base.d.ts

@@ -2,7 +2,11 @@
 // TypeScript Version: 3.2

 import * as React from 'react'
-import { ComponentSelector, Interpolation } from '@emotion/serialize'
+import {


Can revert this change

…tyledopts-arg-be-optional # Conflicts: # packages/native/types/base.d.ts # packages/styled/types/base.d.ts

Andarist · 2022-06-05T07:46:56Z

I will wait for the CI results and I still need to add some changesets here but other than that this is almost ready to land.

If you have any objections to the final shape of this PR - this is the time to raise them. The situation here is quite convoluted and also the ultra-wide type (string) for the shouldForwardProp's parameter is not ideal but given how our types are structured I feel like it's the only way to match type-safety expectations.

srmagura

It looks good to me, though I am not super confident since this is a complex change. My only comments were about cleaning up some of the comments in the code.

srmagura · 2022-06-08T14:50:15Z

packages/native/types/base.d.ts

-/** Same as StyledOptions but shouldForwardProp must be a type guard */
+/**
+ * Same as StyledOptions but shouldForwardProp must be a type guard (https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates).
+ * Practical sense: you can define and reuse atomic `shouldForwardProp` filters that are strictly bound with some `ForwardedProps` type.


See my comments on the version of this in packages/styled/types/base.d.ts.

srmagura · 2022-06-08T14:55:07Z

packages/styled/types/tests.tsx

+  const fc: React.FC<{ foo: string }> = () => null
+
+  // we can't accept a "known" prop here because we need to include `AdditionalProps` and those aren't available yet
+  // `Props` represent the actual props of a component while `AdditionalProps` represent props used only for styling purposes


What is AdditionalProps referring to here? Maybe it is supposed to say ForwardedProps?

AdditionalProps are declared here:

emotion/packages/styled/types/base.d.ts

Line 72 in 65f4444

<AdditionalProps extends {} = {}>(

The problem is this:

styled<GenericsThatDontHaveAccessToAdditionalProps>('div', { shouldForwardProp: (prop: ThisHasToBeAbleToAcceptAdditionalProps) => true })<AdditionalProps>({ color: (props) => props.myAwesomeAdditionalColor })

As we might see here - AdditionalProps are provided to what is being returned from styled but yet shouldForwardProp has only access to what is given to styled.

chore(style): let StyledOptions generic argument be optional

ccc955d

antongolub changed the title ~~chore(style): let StyledOptions generic argument be optional~~ chore(styled): let StyledOptions generic argument be optional Apr 6, 2021

chore(native): let StyledOptions generic argument be optional

29ca437

test(types): add tests for StyledOptions iface

107e017

Andarist reviewed Apr 7, 2021

View reviewed changes

antongolub and others added 4 commits April 7, 2021 20:27

feat(types): add keys assertion for StyledOptions generic

d163c00

fix(styled): add StyledOptions type to CreateStyledComponent call sig…

4c5b17e

…nature fixes emotion-js#2333 (comment)

Merge branch 'main' into let-styledopts-arg-be-optional

659f6ec

Merge branch 'main' into let-styledopts-arg-be-optional

165a422

Andarist reviewed Apr 18, 2021

View reviewed changes

packages/native/types/base.d.ts Outdated Show resolved Hide resolved

Andarist reviewed Apr 18, 2021

View reviewed changes

packages/styled/types/tests.tsx Outdated Show resolved Hide resolved

test(styled): check type StyledOpts for composite component

696213b

antongolub force-pushed the let-styledopts-arg-be-optional branch from d99ba71 to 696213b Compare April 18, 2021 10:50

Andarist reviewed Apr 18, 2021

View reviewed changes

packages/styled/types/base.d.ts Outdated Show resolved Hide resolved

Andarist reviewed Apr 18, 2021

View reviewed changes

packages/styled/types/tests.tsx Outdated Show resolved Hide resolved

antongolub added 2 commits April 19, 2021 09:47

test(styled): revert erroneous changes

fae2dab

test(styled): clarify StyledOptsBroken test case

2238e9e

Merge branch 'main' into let-styledopts-arg-be-optional

a1775f8

antongolub added 2 commits May 29, 2021 18:32

Merge branch 'main' into let-styledopts-arg-be-optional

e36ff17

Merge branch 'main' into let-styledopts-arg-be-optional

009b7b0

Andarist reviewed Feb 25, 2022

View reviewed changes

antongolub added 2 commits March 8, 2022 14:56

chore: use consistent default props type for both StyledOptions and F…

d3092bb

…ilteringStyledOptions

chore: snap emotion-js#3

4104c2a

Andarist reviewed Mar 9, 2022

View reviewed changes

packages/native/types/tsconfig.json Outdated Show resolved Hide resolved

Andarist reviewed Mar 9, 2022

View reviewed changes

packages/styled/types/tsconfig.json Outdated Show resolved Hide resolved

srmagura and others added 3 commits May 21, 2022 10:28

styled.shouldForwardProp: require prop to be a string

5595c8d

Add PropertyKey->string changesets

09df01d

Remove redundant compilerOptions

5f3f5e5

srmagura reviewed May 21, 2022

View reviewed changes

Andarist added 3 commits May 23, 2022 00:23

make shouldForwardProp always contravariant

da3a461

Merge branch 'main' into let-styledopts-arg-be-optional

a4c2a75

Merge branch 'main' into let-styledopts-arg-be-optional

d133a83

Andarist force-pushed the let-styledopts-arg-be-optional branch from 6e308de to cbf28fa Compare May 30, 2022 15:04

Type fixes

f360473

Andarist force-pushed the let-styledopts-arg-be-optional branch from cbf28fa to f360473 Compare May 30, 2022 15:49

Andarist added 4 commits May 30, 2022 18:09

Tweak FilteringStyledOptions and StyledOptions interfaces

ac3f8c7

Change Props constraint

fce092c

Tweak type-level tests around shouldForwardProp

59e8aec

Merge remote-tracking branch 'srmagura/styled-prop-string' into let-s…

65f4444

…tyledopts-arg-be-optional # Conflicts: # packages/native/types/base.d.ts # packages/styled/types/base.d.ts

Andarist mentioned this pull request Jun 5, 2022

styled.shouldForwardProp: require prop to be a string #2759

Closed

srmagura reviewed Jun 8, 2022

View reviewed changes

add changesets

682313b

Andarist merged commit 3055efd into emotion-js:main Jun 12, 2022

github-actions bot mentioned this pull request Jun 12, 2022

Version Packages #2740

Merged

julix-unity mentioned this pull request Nov 13, 2023

Incorrect prop type in shouldForwardProp in emotion 11 #2220

Open

chore(styled): let StyledOptions generic argument be optional #2333

chore(styled): let StyledOptions generic argument be optional #2333

Conversation

antongolub commented Apr 6, 2021

changeset-bot bot commented Apr 6, 2021 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Apr 6, 2021 • edited Loading

Andarist commented Apr 6, 2021

antongolub commented Apr 6, 2021

Andarist commented Apr 6, 2021

antongolub commented Apr 6, 2021

Choose a reason for hiding this comment

antongolub Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antongolub Apr 7, 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

antongolub Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antongolub commented Apr 26, 2021

antongolub commented May 10, 2021

Andarist commented May 10, 2021

antongolub commented Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

antongolub Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antongolub Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

antongolub Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Feb 25, 2022

Andarist commented Mar 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Jun 5, 2022

srmagura left a 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

changeset-bot bot commented Apr 6, 2021 •

edited

Loading

codesandbox-ci bot commented Apr 6, 2021 •

edited

Loading

antongolub Apr 7, 2021 •

edited

Loading

antongolub Apr 7, 2021 •

edited

Loading

antongolub Apr 9, 2021 •

edited

Loading

antongolub commented Jun 11, 2021 •

edited

Loading

antongolub Mar 9, 2022 •

edited

Loading

antongolub Mar 9, 2022 •

edited

Loading

antongolub Mar 9, 2022 •

edited

Loading