-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
chore(styled): let StyledOptions generic argument be optional #2333
Changes from 33 commits
ccc955d
29ca437
107e017
d163c00
4c5b17e
659f6ec
165a422
696213b
fae2dab
2238e9e
a1775f8
e36ff17
009b7b0
b2e0b7a
b9ecb17
1cc73a2
34be1e2
04e367b
310dba8
b8d3039
5286a60
d889b0e
0fa6ee4
83fae39
ebfbd9a
9e78345
894aa3b
43f318b
27b4f2d
c2c2c27
d3092bb
4104c2a
5595c8d
09df01d
5f3f5e5
da3a461
a4c2a75
d133a83
f360473
ac3f8c7
fce092c
59e8aec
65f4444
682313b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,16 +58,19 @@ export type Interpolation< | |
| ArrayInterpolation<MergedProps, StyleType> | ||
| FunctionInterpolation<MergedProps, StyleType> | ||
|
||
/** 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. | ||
*/ | ||
export interface FilteringStyledOptions< | ||
Props, | ||
Props = Record<PropertyKey, any>, | ||
ForwardedProps extends keyof Props = keyof Props | ||
> { | ||
shouldForwardProp?(propName: PropertyKey): propName is ForwardedProps | ||
} | ||
|
||
export interface StyledOptions<Props> { | ||
shouldForwardProp?(propName: PropertyKey): boolean | ||
export interface StyledOptions<Props = Record<PropertyKey, any>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, this generic stays unused so I think it should be more than OK to just drop it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
}
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If you are willing to do the work - then sure, ye, we could try to do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Give me a bit more time. I'll fix the issue above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem - there is no rush. I appreciate your efforts here :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I found the best solution, but it seems working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Andarist, |
||
shouldForwardProp?(propName: keyof Props): boolean | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,11 @@ | |
// TypeScript Version: 3.2 | ||
|
||
import * as React from 'react' | ||
import { ComponentSelector, Interpolation } from '@emotion/serialize' | ||
import { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can revert this change |
||
ComponentSelector, | ||
Interpolation, | ||
InterpolationPrimitive | ||
} from '@emotion/serialize' | ||
import { PropsOf, DistributiveOmit, Theme } from '@emotion/react' | ||
|
||
export { | ||
|
@@ -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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
* Practical sense: you can define and reuse atomic `shouldForwardProp` filters that are strictly bound with some `ForwardedProps` type. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what this line is saying... maybe rewording it or including an example would be helpful. |
||
*/ | ||
export interface FilteringStyledOptions< | ||
Props, | ||
Props = Record<PropertyKey, any>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is just an artifact of the first impl. I'll fix.
It seems that 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 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: PS Sorry for the late reply. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly then with I think you might have a typo in this example as it should be
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.
I'm unsure what's happening here. I understand what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've pushed several snapshot branches so you can observe my poor efforts:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thank you for those - I'll try to review them this week to get a better feeling of what we are dealing with here.
Do we need to care about
Ye, this is impossible - mainly because now you could make such a call So basically, here, assigning to this type is not valid. Such things could be expressed with some function helpers and stuff, like here: 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 |
||
ForwardedProps extends keyof Props = keyof Props | ||
> { | ||
label?: string | ||
shouldForwardProp?(propName: PropertyKey): propName is ForwardedProps | ||
target?: string | ||
} | ||
|
||
export interface StyledOptions<Props> { | ||
export interface StyledOptions<Props = Record<PropertyKey, any>> { | ||
label?: string | ||
shouldForwardProp?(propName: PropertyKey): boolean | ||
shouldForwardProp?(propName: keyof Props): boolean | ||
target?: string | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import * as React from 'react' | ||
import styled from '@emotion/styled' | ||
import styled, { StyledOptions, FilteringStyledOptions } from '@emotion/styled' | ||
|
||
// This file uses the same Theme declaration from tests-base.tsx | ||
|
||
|
@@ -217,3 +217,69 @@ const Input5 = styled.input` | |
// $ExpectError | ||
;<StyledCompWithoutAs as={Section} /> | ||
} | ||
|
||
{ | ||
// Props forwarding through StyledOptions and FilteringStyledOptions | ||
|
||
const fc: React.FC<{ foo: string }> = props => <div {...props} /> | ||
|
||
styled(fc, { shouldForwardProp: (prop: 'foo') => true })({}) | ||
Andarist marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
styled(fc, { shouldForwardProp: (prop: string) => true })({}) | ||
|
||
// $ExpectError | ||
styled(fc, { shouldForwardProp: (prop: 'bar') => true })({}) | ||
|
||
const shouldForwardProp1 = (prop: 'foo') => true | ||
styled(fc, { shouldForwardProp: shouldForwardProp1 })({}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same doubt about this one as here |
||
|
||
const shouldForwardProp2: StyledOptions['shouldForwardProp'] = ( | ||
prop: 'unknown' | ||
) => true | ||
styled(fc, { shouldForwardProp: shouldForwardProp2 })({}) | ||
|
||
const shouldForwardProp3 = (prop: 'unknown') => true | ||
// $ExpectError | ||
styled(fc, { shouldForwardProp: shouldForwardProp3 })({}) | ||
|
||
// $ExpectError | ||
const shouldForwardProp4: StyledOptions<{ | ||
foo: string | ||
}>['shouldForwardProp'] = (prop: 'unknown') => true | ||
|
||
const shouldForwardProp5 = (prop: 'foo'): prop is 'foo' => true | ||
styled(fc, { shouldForwardProp: shouldForwardProp5 })({}) | ||
|
||
const shouldForwardProp6: FilteringStyledOptions['shouldForwardProp'] = ( | ||
prop: 'foo' | ||
): prop is 'foo' => true | ||
|
||
const shouldForwardProp7: FilteringStyledOptions<{ | ||
foo: string | ||
}>['shouldForwardProp'] = (prop: 'foo'): prop is 'foo' => true | ||
|
||
// $ExpectError | ||
const shouldForwardProp8: FilteringStyledOptions<{ | ||
foo: string | ||
}>['shouldForwardProp'] = (prop: 'unknown'): prop is 'unknown' => true | ||
|
||
const shouldForwardProp9: FilteringStyledOptions< | ||
{ foo: string; bar: string }, | ||
'foo' | ||
>['shouldForwardProp'] = (prop: 'foo' | 'bar'): prop is 'foo' => true | ||
|
||
// $ExpectError | ||
const shouldForwardProp10: FilteringStyledOptions< | ||
{ foo: string; bar: string }, | ||
'foo' | ||
>['shouldForwardProp'] = (prop: 'foo' | 'bar'): prop is 'bar' => true | ||
|
||
styled('div', { shouldForwardProp: (prop: 'color') => true })({}) | ||
|
||
styled('div', { | ||
shouldForwardProp: (prop: 'color'): prop is 'color' => true | ||
})({}) | ||
|
||
// $ExpectError | ||
styled('div', { shouldForwardProp: (prop: 'foo') => true })({}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments on the version of this in
packages/styled/types/base.d.ts
.