-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
fix: add fallbacks to jssStyle #1493
base: master
Are you sure you want to change the base?
Conversation
The change seems correct to me, fallbacks are optional, this is waiting for a review from anyone from @cssinjs/typescript team |
Hmmm... At a first glance, the change doesn't appear to be harmful. However, I have seen things behave strangely when something like May I ask what the |
Hmmm. I'm more confident that this won't work now. In the TypeScript playground I tried: type Blah = { pizza: string | number } & { [K: string]: number };
const blah: Blah = {
pizza: "1",
} and got an error because "1" is not a number. Things like Were the tests run for this? |
@ITenthusiasm fallbacks is basically how we describe css fallbacks in js, because we can't use the same property twice with objects https://cssinjs.org/jss-syntax?v=v10.6.0#fallbacks |
This is an example of what I did: |
This change working correct, please review again. |
It would probably be good to have type tests to go with the change. |
Interesting. So if the second type in the |
Yeah. Due to the inconsistencies, I don't think I'm willing to approve this unless there are some robust tests to show nothing unexpected would happen. |
i added few test. |
Please show by example which this change is inconsistent. I removed the second type in the |
@hosseinmd I need to delegate the decision to @ITenthusiasm because he is much deeper than I am in TS |
I add array to test too. |
@ITenthusiasm The idea of |
This example is not correct. You should use |
@hosseinmd I just pulled it and it doesn't compile, I get tons of errors, is it because of recent changes in master? |
Lets finish this one off! |
I rebase master to this branch |
typescript shows errors to me, what am I missing? |
Fixed |
When I run it locally, I see 64 errors
|
@hosseinmd honestly, how did you make it pass tsc compiler ... there are dozens of errors |
Ok I updated to typescript 4.4.2 and now the only 2 errors I get are these: 167 // @ts-expect-error
~~~~~~~~~~~~~~~~~~~
packages/react-jss/tests/types/withStyles.tsx:170:12 - error TS2345: Argument of type '(theme: MyTheme) => Styles<string, unknown, null>' is not assignable to parameter of type 'Styles<string, unknown, MyTheme> | ((theme: MyTheme) => Styles<string, unknown, undefined>)'.
Type '(theme: MyTheme) => Styles<string, unknown, null>' is not assignable to type '(theme: MyTheme) => Styles<string, unknown, undefined>'.
Type 'Styles<string, unknown, null>' is not assignable to type 'Styles<string, unknown, undefined>'.
'string' index signatures are incompatible.
Type 'string | MinimalObservable<string | JssStyle<any, undefined> | null | undefined> | JssStyle<unknown, null> | JssStyle<unknown, null>[] | ((data: { ...; }) => string | ... 2 more ... | undefined)' is not assignable to type 'string | MinimalObservable<string | JssStyle<any, undefined> | null | undefined> | JssStyle<unknown, undefined> | JssStyle<unknown, undefined>[] | ((data: unknown) => string | ... 2 more ... | undefined)'.
Type '{ fallbacks?: JssStyle<unknown, null> | JssStyle<unknown, null>[] | undefined; } & { alignContent?: AlignContent | MinimalObservable<AlignContent | JssStyle<any, undefined> | undefined> | JssStyle<...> | ((data: { ...; }) => AlignContent | ... 1 more ... | undefined) | undefined; ... 763 more ...; vectorEffect?: Vec...' is not assignable to type 'string | MinimalObservable<string | JssStyle<any, undefined> | null | undefined> | JssStyle<unknown, undefined> | JssStyle<unknown, undefined>[] | ((data: unknown) => string | ... 2 more ... | undefined)'.
Type '{ fallbacks?: JssStyle<unknown, null> | JssStyle<unknown, null>[] | undefined; } & { alignContent?: AlignContent | MinimalObservable<AlignContent | JssStyle<any, undefined> | undefined> | JssStyle<...> | ((data: { ...; }) => AlignContent | ... 1 more ... | undefined) | undefined; ... 763 more ...; vectorEffect?: Vec...' is not assignable to type '{ fallbacks?: JssStyle<unknown, undefined> | JssStyle<unknown, undefined>[] | undefined; } & { [K: string]: JssValue | MinimalObservable<JssValue | JssStyle<any, undefined> | undefined> | JssStyle<...> | ((data: unknown) => JssValue | ... 1 more ... | undefined); }'.
Type '{ fallbacks?: JssStyle<unknown, null> | JssStyle<unknown, null>[] | undefined; } & { alignContent?: AlignContent | MinimalObservable<AlignContent | JssStyle<any, undefined> | undefined> | JssStyle<...> | ((data: { ...; }) => AlignContent | ... 1 more ... | undefined) | undefined; ... 763 more ...; vectorEffect?: Vec...' is not assignable to type '{ fallbacks?: JssStyle<unknown, undefined> | JssStyle<unknown, undefined>[] | undefined; }'.
Types of property 'fallbacks' are incompatible.
Type 'JssStyle<unknown, null> | JssStyle<unknown, null>[] | undefined' is not assignable to type 'JssStyle<unknown, undefined> | JssStyle<unknown, undefined>[] | undefined'.
Type '{ fallbacks?: JssStyle<unknown, null> | JssStyle<unknown, null>[] | undefined; } & { alignContent?: AlignContent | MinimalObservable<AlignContent | JssStyle<any, undefined> | undefined> | JssStyle<...> | ((data: { ...; }) => AlignContent | ... 1 more ... | undefined) | undefined; ... 763 more ...; vectorEffect?: Vec...' is not assignable to type 'JssStyle<unknown, undefined> | JssStyle<unknown, undefined>[] | undefined'.
170 withStyles(passingFunctionNullTheme)(SimpleComponent)
~~~~~~~~~~~~~~~~~~~~~~~~
Found 2 errors. I have no clue why they happen and its frustrating up to the point to throw ts out altogether and make it someone else's problem. |
I pushed my state to this branch: https://github.com/cssinjs/jss/tree/fallbacksType would appreciate help @cssinjs/typescript |
Oh those 2 errors happen on master too when upgrading ts to 4.4.2, maybe @ITenthusiasm knows what that is |
we should discuss it in this separate PR #1550 |
Corresponding Issue(s):
What Would You Like to Add/Fix?
Todo
Expectations on Changes
Changelog