styled-components: Add more complete support for object styles, tests #30511
Conversation
|
@Kovensky Thank you for submitting this PR! 🔔 @Igorbek @Igmat @lavoaster - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
types/styled-components/index.d.ts
Outdated
| > = | ||
| // at least the first argument is required, whatever it is | ||
| <U extends object>( | ||
| first: NonNullable< |
There was a problem hiding this comment.
Testing all these functions at runtime; if the first argument is missing or undefined, styled-components will throw by trying to index it. If it is null, styled-components will try to call it as a function 😆
Anyway, first interpolation is mandatory.
There was a problem hiding this comment.
Oh, this doesn't break usage as a template string because both Interpolation and SimpleInterpolation accept TemplateStringsArray (effectively string[] & { raw: string[] }) by being assignable to ReadonlyArray<string>.
Let me know if you want it to still be left there explicitly.
| | FlattenInterpolation<P>; | ||
| // must be an interface to be self-referential | ||
| export interface FlattenInterpolation<P> | ||
| extends ReadonlyArray<Interpolation<P>> {} |
There was a problem hiding this comment.
They can be arbitrarily nested.
> sc.css([[[[[[['color: green;']]]]]]])
[ 'color: green;' ]
| | FlattenSimpleInterpolation; | ||
| // must be an interface to be self-referential | ||
| interface FlattenSimpleInterpolation | ||
| extends ReadonlyArray<SimpleInterpolation> {} |
There was a problem hiding this comment.
Same with SimpleInterpolation
> sc.keyframes([[[[[[['from { color: green; }']]], { to: { color: 'red' } }]]]])
Keyframes {
inject: [Function],
toString: [Function],
name: 'ijNbFt',
rules:
[ '@-webkit-keyframes ijNbFt{from{color:green;}to{color:red;}}',
'@keyframes ijNbFt{from{color:green;}to{color:red;}}' ],
id: 'sc-keyframes-ijNbFt' }
However, keyframes absolutely cannot take any function. It'll just call .toString() on it and add to the rules.
| export type ThemedGlobalStyledClassProps<P, T> = P & { | ||
| suppressMultiMountWarning?: boolean | ||
| theme?: T | ||
| export type ThemedGlobalStyledClassProps<P, T> = WithOptionalTheme<P, T> & { |
There was a problem hiding this comment.
This wasn't excluding any theme that might have been expected by interpolations, so it'd be kept as a required prop.
types/styled-components/index.d.ts
Outdated
| StyledComponent<any, any>, | ||
| keyof StyledComponent<any, any> | ||
| AnyStyledComponent, | ||
| keyof StyledComponentBase<any, any> |
There was a problem hiding this comment.
Taking the keyof StyledComponentBase to get rid of everything in interface String {}...
| A extends keyof any = never | ||
| > = // the "string" allows this to be used as an object key | ||
| // I really want to avoid this if possible but it's the only way to use nesting with object styles... | ||
| string & StyledComponentBase<C, T, O, A>; |
There was a problem hiding this comment.
This hack was needed to allow this:
const StyledDiv = styled.div``
css({
background: 'white',
[StyledDiv]: {
background: 'black'
}
})This works because styled components have a toString() implementation, but just adding a toString(): string to the interface is not sufficient to make typescript like it as an object key.
types/styled-components/index.d.ts
Outdated
| export type BaseThemedCssFunction<T extends object> = <P>( | ||
| first: NonNullable<Interpolation<ThemedStyledProps<P, T>>>, | ||
| ...interpolations: Array<Interpolation<ThemedStyledProps<P, T>>> | ||
| ) => Array<FlattenInterpolation<ThemedStyledProps<P, T>>>; |
There was a problem hiding this comment.
The result is indeed always an array.
| keyframes( | ||
| strings: TemplateStringsArray, | ||
| strings: TemplateStringsArray | CSSKeyframes, | ||
| ...interpolations: SimpleInterpolation[] | ||
| ): Keyframes; |
There was a problem hiding this comment.
This kinda doesn't belong here as it can't be affected by the type of theme anyway...... but on the other hand, omitting it would make it harder to use, with you having to import keyframes from elsewhere instead of from your strongly typed wrapper.
| * without needing to reexport `ThemedStyledComponentsModule`. | ||
| */ | ||
| // Unfortunately, there is no way to write tests for this | ||
| // as any augmentation will break the tests for the default case (not augmented). | ||
| // tslint:disable-next-line:no-empty-interface | ||
| export interface DefaultTheme {} |
There was a problem hiding this comment.
I couldn't find any way to test this in an isolated context that wouldn't affect the type of DefaultTheme as seen from index.tsx. It's very important for the existing tests that DefaultTheme be empty...
However, augmenting DefaultTheme did cause expected type errors so it seems to work 🤷♀️
…afe ref, and as (#30467) * Add back context; add more helper context types and documentation * Fix the handling of ref to properly exclude string refs where not supported * Fix erroneous tests that got caught by the better Ref type * Augment ComponentProps to support JSX.IntrinsicElements * Add tests, doc comments for ComponentProps* * Add self to Definitions by * Add TODO comment * Add comments about the "'ref' extends keyof P" checks * Fix the handling of ref and props when using withComponent in styled-components * Swap nesting order of mapped types to fix missing $ExpectErrors * Comment out test that fails in [email protected] * Hack to deal with the tests not failing when they should * Rewrite styled-components * Bump version * Bump minimum TypeScript version * Update types of @rebass/grid (depends on styled-components) * Update styled-react-modal types to 1.1 (also styled-components related) * Reformat styled-components's index.d.ts
|
Trying this out in a full codebase instead of just the tests...... there is an annoying amount of places where the generic inference stops working and you have to pass an explicit generic argument to There is one fix I need to make to the type of
Giving it directly as a template argument using Oh...... it seems that, because I have no idea why, React.ReactElement is not a valid Interpolation. I also managed to fix that, but the fix for that broke inference for the I also have no idea why 😢 More EDIT: I was accidentally returning an array array in |
|
This is probably good for review |
typescript-bot
added
the
Popular package
|
Awesome work, I hope I find time to try it out on my code base this weekend. |
|
I had the same issue as @joealden as described in 5c49d4d#commitcomment-31305259, and I can confirm this PR fixed the problem, thanks ! |
typescript-bot
added
the
Unmerged
|
After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience! |
npm test.)npm run lint package-name(ortscif notslint.jsonis present).