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
[react-jss] Version 10.0.3 contains breaking change in Typescript definitions #1255
Comments
Long time, no answer. Are you even gonna address this issue? |
@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. |
From what I saw in the code, that was specifically typing issue. js code still accepts second argument |
@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:
|
@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. |
Is there any workaround to make Typescript happy (apart from use the version 10.0.2)? |
After upgrading to
And also got the same errors as the creator of this issue. E.g:
Not sure if it relates only to global styles, but such breaking changes stop all work and forced to revert to previous versions. |
I really don't understand what the fuss is about. Just revert to old types which were good enough, release it as a |
fixed in 10.0.4 |
Thanks! 🙏 |
@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 |
@BEGEMOT9I @HenriBeck is focusing on TS types. |
But maybe we could do it just for the sake of "better" types and not "100% correct types" cc @HenriBeck ? |
@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) |
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. |
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. |
@BEGEMOT9I I checked today and for me, the auto-completion also worked when using |
@HenriBeck Yes, without listing all the properties in the generator signature. |
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 ***@***.***> 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
<#1255?email_source=notifications&email_token=AAAM4WC2IAKE44U437PJJQDRACEPBA5CNFSM4KDDKWP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKEVNCI#issuecomment-579425929>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAM4WECI53U3VXTRTYUTC3RACEPBANCNFSM4KDDKWPQ>
.
|
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')]: {}
}) |
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. |
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. |
Yes exactly |
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 |
But in this case everything will be okay, because the |
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 |
In the case of using my version, I will only need to convey only once the entity that I want to describe. |
Could you please provide an example using your definition? You can’t solve the issue when using dynamic media queries |
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. |
So, after upgrading to
And I receive:
|
@HenriBeck btw, in the current version of |
Was about to make a new issue but I think this is related:
Get this error when trying to assign a flexDirection in styles:
This really confuses me:
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:
and
But neither of those work. |
fix now rereleased in v10.3.0, see conversation #1352 |
Here is TS's opinion on breaking changes DefinitelyTyped/DefinitelyTyped#25677 (comment) |
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:Thus user could use types styles with provided theme as follows:
And the
useStyles
hook would return proper classNames.Describe the bug:
In react-jss@10.0.3 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.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):
Feel free to add any additional versions which you may think are relevant to the bug.
The text was updated successfully, but these errors were encountered: