Upgrade typescript to 4.4.2

[kof]

This is failing atm with 2 errors, need help with this

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.

[ITenthusiasm]

I'm preoccupied at the moment, but I can try to look at it in the next day or 2. (If I don't, feel free to ping me again.) I think I remember that test getting finicky. My hunch is that it should be simple to handle.

I don't have the code in front of me right now. But more than likely, the test is trying to assert something that is no longer true in 4.4.2. (Meaning this is not necessarily a "TS error" per se. But again, I'd have to have the code in front of me to be sure.)

[kof]

I think you are definitely right and my question is more what was the original goal and what do we do now since this isn't an error.

[ITenthusiasm]

@kof Mkay I see. The issue is with the attempted failing cases in withStyles. I think these range from lines 137 onwards.

Background

Basically, the tests are to verify that a user cannot supply conflicting themes. If a Theme type is supplied as an argument to the function, then a separate theme type should not be supplied within the returned Styles type. Our TS definitions try to assert this by saying that if a function is passed to withStyles, then the Theme on Styles will be undefined (since it isn't needed or used).

Ideally, a user wouldn't have to bother worrying about this, as they could simply omit the third type parameter of Styles and go on about their day. However, to prevent any unforeseen issues, these tests were created.

It seems between the migration to TS and the upgrading of its package, some stuff broke -- in a good way, I think. any is indeed any. So technically speaking, it should be assignable to undefined. I'm not sure what's going on with unknown, but it's a weird use case to test anyway. null breaking makes sense because null !== undefined.

Solution

Instead of trying to pass abstract types that are basically any and any-ish (unknown), I think we should work with actual types.

Under the Failing Cases section, we can remove the any test. And we can replace the unknown test with the test of an empty object ({}). This will should be failing case.

The null case is already a failing case, so we just have to appropriately place an @ts-expect-error overtop.

The functions in this fail-case section should start with failing, not passing. (We should apply any other renaming deemed appropriate as well.)

[kof]

Oh man those ts errors I get while trying to fix things are completely unreadable ... I don't know what to do with this entire typescript story. Anybody who will get such errors as a user will hate JSS.

[kof]

We need a complete rewrite of typescript definitions or even of the API to make those typescript errors readable.

[kof]

What do you think are the core reasons for such unreadable meaningless errors that come from ts in this API?

[kof]

When I try to use the correct theme as a 3rd argument, I still get an error and I have no idea why.

function failingFunctionWrongTheme(theme: MyTheme): Styles<string, unknown, MyTheme> {

[kof]

Can we actually safely upgrade to typescript 4 in a minor version?

[ITenthusiasm]

Can we actually safely upgrade to typescript 4 in a minor version?

It looks like we didn't have to change any interfaces. So it should be safe, yes. Users shouldn't even notice a difference.

[ITenthusiasm]

When I try to use the correct theme as a 3rd argument, I still get an error and I have no idea why.

function failingFunctionWrongTheme(theme: MyTheme): Styles<string, unknown, MyTheme> {

Sorry, I'm catching up to all of these late.

Yeah there's an error here because the type that we have right now is trying to prevent any kind of overriding. In retrospect, it might've been better to allow the original type (or otherwise use never). That might make more sense to people. But even so, the third type parameter still won't be used because TS is pulling from the argument passed to the function.

Edit: If we update the type, that should probably be a separate PR though. And TS could be upgraded regardless.

[ITenthusiasm]

What do you think are the core reasons for such unreadable meaningless errors that come from ts in this API?

My first guess would be the need to use recursive types. Plus the fact that the recursive type itself is very complex. The objects and comparisons likely go so deep that the printed error gets too unwieldy.

Edit: However, looking at the error you posted earlier:

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>'.

If someone just stops at that last line, the error should hopefully become obvious. The cause of the error is right there. The unnecessary, deep details of that error are what follow afterwards. Some people in the TS world just learn how to pick out the most helpful pieces of information from a TS type error. It comes with time. (That still leaves the question open of whether or not it's a concern.)

[kof]

Is there a way to make errors better and avoid recursion?

[kof]

I also see that typescript is struggling performance-wise, can recursion be the reason for that as well?

[ITenthusiasm]

Honestly, I'm not sure. :pensive: If I understand correctly, JSS needs to allow object shapes that basically resemble recursive types, right? So there may not be a way around this. There might be a way to make the recursive type cleaner if possible. But that's probably the best we could do.

[kof]

released in https://github.com/cssinjs/jss/releases/tag/v10.8.0