fix createUseStyles types

[moshest]

Corresponding issue (if exists):

#1351

What would you like to add/fix?

This PR re-add TypeScript support for createUseStyles by redefining JssStyle recursively. This change enables IDEs to autocomplete known CSS properties and also allow unknown properties as well. In addition, it fixes the return type for function value and function rules, making both accept null, false or undefined values to disable the rule/class.

Todo

• Add tests if possible - new test file under packages/jss/tests/types/Styles.ts
• Add changelog if users should know about the change
• Add documentation

[moshest]

I don't think this test failure is related to my changes..

09 06 2020 21:25:16.741:ERROR [launcher]: Cannot load browser "BS_Safari Yosemite"!
440  Error: Password is required.

[kof]

That's because we are not able to run browserstack tests for external contributors

[kof]

Is this a breaking change for the user?

[moshest]

Don't think so. Types should never trigger breaking changes.

That's said, I notice some new errors on my code if I wrote a function that return undefined instead of null (as any !== JssValue). Not sure it's a breaking change, fells more like a fix.

If that not a fix and JSS support also undefined values, we should update JssValue as well and add undefined to the list.

Other then that, it should accept basically anything, including custom css properties like:

const useStyles = createUseStyles({
  root: {
   fontSize: 12,
   customFont: 'foo' // <- non-existing property
  }
});

[kof]

Types should never trigger breaking changes.

#1255 (comment)

[kof]

If someone suddenly can't compile types after updating a minor change, how is this not breaking users code?

[moshest]

I think that if the user did something wrong and the type check suddenly catch that on a version update it's a good thing. It's better to failed on compile time then on runtime.

As far as I know, those types are align with the current implementation. The only contradiction that I found is on the JssValue type and it's not directly related to this fix.

It's seems that JSS works with undefined values but it's not allowed on the TS types. So the question is, do we want to allow undefined as a valid JssValue or it's consider a bug?

[kof]

Falsy values are supported for the rule function (function returning an object with properties/values)
https://github.com/cssinjs/jss/blob/master/packages/jss-plugin-rule-value-function/src/index.js#L58

But https://github.com/cssinjs/jss/blob/master/packages/jss/src/plugins/styleRule.js#L45 method styleRule.prop can set and get in one method and so if the second argument is undefined, it will act as a getter, so it turns out we have an inconsistency with function values https://github.com/cssinjs/jss/blob/master/packages/jss-plugin-rule-value-function/src/index.js#L75 because if fnValues[prop](data) returns an undefined styleRule.prop(prop, fnValues[prop](data), options) will act as a getter.

To make it consistent we need to support falsy values here

[kof]

I think that if the user did something wrong and the type check suddenly catch that on a version update it's a good thing. It's better to failed on compile time then on runtime.

2 questions:

1. Did user had actually wrong code that will now be uncovered in this case?
2. Should upgrading a non-major version result in any code changes require by the user? In theory no, even if user's code was wrong already. It was tested the way it was written and considered OK. Now if user needs to change their code in a minor upgrade, they will have to go through verification process and I am not sure this is expected from a minor upgrade.

[kof]

The thing is any change is potentially a breaking change. Semver is here to communicate to the user: "you will have to change your code and verify it works".

[moshest]

I don't want to change functionality in this PR. If undefined is only supported on functions I can fix the types to support that only:

export type JssStyle = {
  [K in keyof NormalCssProperties | string]:
    | NormalCssValues<K>
    | JssStyle
    | ((data: any) => NormalCssValues<K> | JssStyle | undefined)
}

To answer your questions:

1. Yes, following your comment it's seems that user did something wrong if he provide undefined to styleRule.prop and this PR will catch that.
2. Since we didn't change any functionally in this PR, i don't think we need to bump the version and let the user recheck his code. If we will get some errors from the user about failed type check that actually work on runtime we can release another fix to address that but I idea is that it shouldn't happen.

[kof]

I don't want to change functionality in this PR. If undefined is only supported on functions I can fix the types to support that only:

In this case right now we support falsy values for function rules and only a null for function values.

[moshest]

I don't want to change functionality in this PR. If undefined is only supported on functions I can fix the types to support that only:

In this case right now we support falsy values for function rules and only a null for function values.

What's the different between "function rules" and "function values"? It's not clear to me..

[kof]

https://cssinjs.org/jss-plugin-rule-value-function

[kof]

Regarding breaking changes in types: https://twitter.com/FezVrasta/status/1270648268012339203?s=20 argument resonates with me

In theory, it should have been breaking change, but both ts and flow are a mess in terms of breaking changes so we shouldn't care either breaking users types in minor versions.

[moshest]

I think now we align with the docs.

[kof]

I think we should align with how it is supposed to be used: both null and undefined returned from both function rule and function value should result in removing that rule or value

[moshest]

OK, Done.

[kof]

Alright, lets try releasing it as a minor change and see what happens. Can you please write a summary in the description of this PR and very short one in the changelog after ---? I have hard times explaining what this PR does lol and users will come for it. Maybe even as part of this PR description a migration strategy?

[moshest]

Done

[kof]

Here is TS's opinion on breaking changes DefinitelyTyped/DefinitelyTyped#25677 (comment)

[kof]

released in v10.3.0

[moshest]

Just tested on large project, all types working fine.

[afzalsayed96]

I just upgraded a typescript project from 10.1.1 to 10.4.0 for a bug fix and a ton of shit broke 😔

[kof]

@afzalsayed96 sorry to hear that, you will have to report separate issues, also check out the changelog between the versions

[afzalsayed96]

Any plans to support falsy values in function values? I'm widely using something like below pattern everywhere in my codebase:

block: ({ height, width }: { height?: number, width?: number }) => ({
    height,
    width
  })

[moshest]

@afzalsayed96 can you attach the typescript errors you get (preferably on a new issue, linked to this one + tag me)?