Jss: [react-jss] Version 10.0.3 contains breaking change in Typescript definitions

[pajasevi]

__Expected behavior:__
In version 10.0.0 and above, the TS definition clearly states that, if user wants to use theme in the function, Theme type must be supplied to the generic createUseStyles type as follows:

declare function createUseStyles<T, C extends string>(
  styles: (theme: T) => Record<C, Style | string>,
  options?: {
    index?: number
    name?: string
    theming?: Theming<T>
  } & StyleSheetFactoryOptions
): (data?: any) => Record<C, string>

Thus user could use types styles with provided theme as follows:

const useStyles = createUseStyles<Theme, "checkboxWrapper" | "input">((theme: Theme) => ({
    checkboxWrapper: {
        display: "inline-flex",
        alignItems: "center",
        fontSize: theme.utils.pxToRem(14),
    },
    input: {
        marginRight: theme.utils.pxToRem(6),
    }
}));

And the useStyles hook would return proper classNames.

__Describe the bug:__
In [email protected] a new typescript definitions were introduced which are breaking against previous behavior. The Theme argument no longer needs to be supplied to the generic which means that every usage of createUseStyles is now broken.

declare function createUseStyles<C extends string = string>(
  styles: ((theme: any) => Styles<C>),
  options?: CreateUseStylesOptions
): (data?: unknown) => Classes<C>

__Recommended fix__
Revert this behavior under 10.0.4 version and release a new major version with this breaking change, according to the semver.

__Codesandbox link:__
Please create a codesandbox.io with the issue. Make it as minimal as possible as this will help us find the bug quicker.

__Versions (please complete the following information):__

• react-jss: 10.0.3
• Browser [e.g. chrome, safari]: yes
• OS [e.g. Windows, macOS]: yes
  Feel free to add any additional versions which you may think are relevant to the bug.

[pajasevi]

Long time, no answer. Are you even gonna address this issue?

[HenriBeck]

@kof Do you think this is worth a completely new major version? This is only breaking change about types (Generics) (not actually running code) and I personally don't think it should be hard to migrate for most users.

[yhaskell]

From what I saw in the code, that was specifically typing issue. js code still accepts second argument

[pajasevi]

@HenriBeck You either follow semver and release a major version because it contains breaking changes (no matter it's "only in types") or you don't. And according to semver, the new patch version, which 10.0.3 is, is specified as:

Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

[kof]

@HenriBeck semver is not objective, it's mostly a tool to communicate the intent to the user. Objectively every change is a breaking change. So if it causes breaking behavior to users, it should be a major version bump.

[BEGEMOT9I]

May be we could use another version?
Declaration:

declare function createUseStyles<S>(
  styles: S,
  options?: CreateUseStylesOptions
): (data?: unknown) => S extends ((theme: any) => any)
  ? Classes<keyof ReturnType<S>>
  : Classes<keyof S>

Use-case (styles.ts):

import { createUseStyles } from 'react-jss'

import { Theme } from 'lib/theme'

const styles = {
  container: {
    backgroundColor: 'green'
  }
}
const themedStyles = (theme: Theme) => ({
  ...styles,
  container: {
    ...styles.container,
    backgroundColor: theme.hookColor
  }
})
const useWithoutThemeStyles = createUseStyles<typeof styles>(styles)
const useWithThemeStyles = createUseStyles<typeof themedStyles>(themedStyles)

Use-case (index.tsx):

import React, { FC } from 'react'

import { useWithThemeStyles, useWithoutThemeStyles } from './styles'

interface OuterProps {
  withTheming: boolean
}
interface Props extends OuterProps {}

const Item: FC<Props> = ({ withTheming }) => {
  const classesWithTheme = useWithThemeStyles()
  const classesWithoutTheme = useWithoutThemeStyles()
  const classes = withTheming ? classesWithTheme : classesWithoutTheme

  return <div className={classes.container}>Hook{withTheming && ' (with theme)'}</div>
}

export default Item

In this implementation, we immediately have the correct typing for the classes and auto-completion:

[filosganga]

Is there any workaround to make Typescript happy (apart from use the version 10.0.2)?

[vtereshyn]

After upgrading to 10.0.3 I receive tons of errors for my global styles like:

'*': {
   boxSizing: 'border-box'
},

html: {
  fontSize: '62.5%'
},

And also got the same errors as the creator of this issue.

E.g:

 Property ''.mainWrapper'' is incompatible with index signature.                                                                                 
            Type '{ backgroundColor: string; minHeight: string; display: string; flexDirection: string; }' is not assignable to type 'FnValue<string | number | f
alse | (string | number | (string | number)[])[] | JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<void>>>>>> | null>'.                              
              Type '{ backgroundColor: string; minHeight: string; display: string; flexDirection: string; }' is not assignable to type 'JssStyleP<JssStyleP<JssSt
yleP<JssStyleP<JssStyleP<JssStyleP<void>>>>>>'.                                                                                                                  
                Type '{ backgroundColor: string; minHeight: string; display: string; flexDirection: string; }' is not assignable to type 'CssProperties'.        
                  Types of property 'flexDirection' are incompatible.                                                                                            
                    Type 'string' is not assignable to type 'FnValue<"column" | "inherit" | "-moz-initial" | "initial" | "revert" | "unset" | "column-reverse" | 
"row" | "row-reverse" | undefined>'

Not sure if it relates only to global styles, but such breaking changes stop all work and forced to revert to previous versions.

[pajasevi]

I really don't understand what the fuss is about. Just revert to old types which were good enough, release it as a 10.0.4 (because that would actually be a bug fix) and release new types as major version.

[kof]

fixed in 10.0.4

[pajasevi]

Thanks! 🙏

[BEGEMOT9I]

@kof why we couldn't use solution, that i mentioned before? In this case, we do not need to list all the properties of the declaration (of which there may be a sufficiently large number) and at the same time we retain the possibility of auto-completion

[kof]

@BEGEMOT9I @HenriBeck is focusing on TS types.
I looked into your example, you are making an assumption that styles keys are always the same as classes keys, this is not quite correct for media for example.

[kof]

But maybe we could do it just for the sake of "better" types and not "100% correct types" cc @HenriBeck ?

[BEGEMOT9I]

@kof Yes, it is, in this case typing is more important in terms of support for auto-completion. To implement the omitting, we could add one more parameter:

// index.d.ts
declare function createUseStyles<S, OmitRules extends string = ''>(
  styles: S,
  options?: CreateUseStylesOptions
): (data?: unknown) => S extends ((theme: any) => any)
  ? Omit<Classes<keyof ReturnType<S>>, OmitRules>
  : Omit<Classes<keyof S>, OmitRules>

// styles.ts
const styles = {
  container: {
    margin: '0.125rem',
    padding: '0.5rem',
    fontSize: '0.75rem',
    color: '#fff',
    backgroundColor: 'green',
    borderRadius: '0.25rem'
  },
  'media (max-width: 800px)': {
    container: {
      margin: '1.125rem'
    }
  }
}
const useWithoutThemeStyles = createUseStyles<typeof styles, 'media (max-width: 800px)'>(styles)

[kof]

In this case you would put it on the user to add OmitStrings and considered media queries are often containing some variable interpolations it's a pretty bad DX in general, but I don't see a good way either except of not using root level media queries and only using them inside of rules. Also it's not just media rule, it's a bunch of other at rules too.

[BEGEMOT9I]

Ok. That is, in the end, you need to decide whether it makes sense to deceive the user with an incorrect typing? As an active user of this library, I often have 5-10 different classes / properties inside the styles object in work projects. And every time, for typing to work, I need to list them all, or forget about autocompletion and recognize when rendering the component that I made a spelling mistake in the class name.
P.S. This is not a claim at all, just a working moment

[HenriBeck]

@BEGEMOT9I I checked today and for me, the auto-completion also worked when using createUseStyles. Or what is the exact problem?

[BEGEMOT9I]

@HenriBeck Yes, without listing all the properties in the generator signature.
The problem is that for each new classname it is necessary to add a new property with the second parameter. And this must be done every time a new classname appears. Therefore, in our projects, we have a rewritten hook signature (as in the example above), where the autocomplete remains.

[kof]

Haven't had time to look into it but aparently there is another way

https://twitter.com/oleg008/status/1222188611078119425?s=19

On Tue, Jan 28, 2020, 20:54 BEGEMOT9I [e-mail] wrote:

@HenriBeck https://github.com/HenriBeck Yes, without listing all the
properties in the generator signature.
The problem is that for each new classname it is necessary to add a new
property with the second parameter. And this must be done every time a new
classname appears. Therefore, in our projects, we have a rewritten hook
signature (as in the example above), where the autocomplete remains.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/cssinjs/jss/issues/1255?email_source=notifications&email_token=AAAM4WC2IAKE44U437PJJQDRACEPBA5CNFSM4KDDKWP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKEVNCI#issuecomment-579425929,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAAM4WECI53U3VXTRTYUTC3RACEPBANCNFSM4KDDKWPQ
.

[BEGEMOT9I]

Yeah, thanks, it's fixed some cases, like static media queries. But we need to avoid a calculated mq, for example (from material):

const styles = (theme) => ({
  ...,
  [theme.breakpoints.up('md')]: {}
})

[HenriBeck]

In the dynamic media query case, there is nothing we can do about it. What you can do is put the media query inside the class itself, so not on the top level. Then the class name type shouldn’t be widened to a string.

[BEGEMOT9I]

Then the class name type shouldn’t be widened to a string

You talking about a such case:

const styles = (theme) => ({
  container: {
    ...,
    [theme.breakpoints.up('md')]: {}
  },
})

In this case, the classname should still be in the resulting type.

[HenriBeck]

Yes exactly

[BEGEMOT9I]

As a result, I personally advocate transferring the type of object / function to the generic and have extra properties in the form of mq and etc, which I will not use anyway. You can write a basic Omit, in which, for example, there will already be basic keywords, such as @global.

[BEGEMOT9I]

Yes exactly

But in this case everything will be okay, because the container property will be in the resulting type. And it should be there.

[HenriBeck]

What kind of benefit would having the style object in the generic? We wouldn’t be able to remove the media queries anyways, especially with computed ones

[BEGEMOT9I]

1) I do not need to list all the classes that I want to use
2) When modifying a stylized object, I will need to modify the generic as well. In this case, I must follow the spelling of the class key

In the case of using my version, I will only need to convey only once the entity that I want to describe.

[HenriBeck]

Could you please provide an example using your definition? You can’t solve the issue when using dynamic media queries

[BEGEMOT9I]

I do not propose to solve the problem of excluding ALL calculated mq. They will still be "hidden" from the developer in the autocompletion interface, since they cannot be accessed by key. I propose to exclude the basic constructions, about which I already wrote above, and, perhaps, try to exclude static mq, if there is an implementation option of what the @kof suggested using the regexp.

[vtereshyn]

So, after upgrading to 10.0.4 I don't see if issue with types has been fixed:

const styles = {
  verticalDoubleContainer: {
    width: '100%',
    position: 'relative',

    '& .recharts-cartesian-axis-ticks text tspan': {
      textTransform: 'uppercase',
      fill: '#6a7175',
      fontSize: '1rem',
      letterSpacing: '1px'
    }
  }
};

type Props = WithStylesProps<typeof styles> &  {}

And I receive:

 Argument of type '{ verticalDoubleContainer: { width: string; position: string; '& .recharts-cartesian-axis-ticks text tspan': { textTransform: string; fill: string; fontSize: string; letterSpacing: string; }; }; barLabel: { ...; }; barTitle: { ...; }; }' is not assignable to parameter of type 'Record<string | number | symbol, string | JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<void>>>>>>>> | ((theme: any) => Record<...>)'.
  Type '{ verticalDoubleContainer: { width: string; position: string; '& .recharts-cartesian-axis-ticks text tspan': { textTransform: string; fill: string; fontSize: string; letterSpacing: string; }; }; barLabel: { ...; }; barTitle: { ...; }; }' is not assignable to type 'Record<string | number | symbol, string | JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<void>>>>>>>>'.
    Property 'verticalDoubleContainer' is incompatible with index signature.
      Type '{ width: string; position: string; '& .recharts-cartesian-axis-ticks text tspan': { textTransform: string; fill: string; fontSize: string; letterSpacing: string; }; }' is not assignable to type 'string | JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<void>>>>>>>'.
        Type '{ width: string; position: string; '& .recharts-cartesian-axis-ticks text tspan': { textTransform: string; fill: string; fontSize: string; letterSpacing: string; }; }' is not assignable to type 'JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<JssStyleP<void>>>>>>>'.
          Type '{ width: string; position: string; '& .recharts-cartesian-axis-ticks text tspan': { textTransform: string; fill: string; fontSize: string; letterSpacing: string; }; }' is not assignable to type 'CssProperties'.
            Types of property 'position' are incompatible.
              Type 'string' is not assignable to type 'FnValue<"relative" | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | "fixed" | "-webkit-sticky" | "absolute" | "static" | "sticky" | undefined>'.

136 export default withStyles(styles)(ResponsiveBar);

[BEGEMOT9I]

@HenriBeck btw, in the current version of WithStylesProps realized the same logic, that i supposed.

[reifnotreef]

Was about to make a new issue but I think this is related:

(JSX attribute) HTMLAttributes<HTMLDivElement>.style?: React.CSSProperties
Type '{ backgroundColor: string; display: string; flex: string; flexDirection: string; }' is not assignable to type 'CSSProperties'.
  Types of property 'flexDirection' are incompatible.
    Type 'string' is not assignable to type 'FlexDirectionProperty'.ts(2322)
index.d.ts(1763, 9): The expected type comes from property 'style' which is declared here on type 'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>'

Get this error when trying to assign a flexDirection in styles:

const styles = {
  root: {
    backgroundColor: 'red',
    display: 'flex',
    flex: 'row',
    flexDirection: 'row',
  },
};

This really confuses me:

Types of property 'flexDirection' are incompatible.
            Type 'string' is not assignable to type '"row" | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | "column" | "column-reverse" | "row-reverse" | PropsFunc<{}, FlexDirectionProperty>'.ts(2345)

It doesn't know that the string 'row' is === "row"

single quotes are being put in place via eslint/prettier, even when I try using double quotes it doesn't change it. I have also tried nested flex properties in an object:

flex : {
  direction: 'row',
}

and

flex: {
  flexDirection: 'row',
},

But neither of those work.
@kof

[kof]

fix now rereleased in v10.3.0, see conversation https://github.com/cssinjs/jss/pull/1352

[kof]

Here is TS's opinion on breaking changes https://github.com/DefinitelyTyped/DefinitelyTyped/issues/25677#issuecomment-388215959