Skip to content
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

Keep classes ref when sheet and dynamicRules have not any change #1573

Conversation

behnammodi
Copy link
Member

@behnammodi behnammodi commented Oct 26, 2021

What Would You Like to Add/Fix?

getSheetClasses always return new object even sheet and dynamicRules haven't any change

Also I wrote a small test to keep it in future

Todo

  • Add test(s) that verify the modified behavior

Expectations on Changes

So, I memoized it to prevent this behavior

Changelog

Improve performance at useStyle

@behnammodi behnammodi requested a review from kof as a code owner October 26, 2021 06:30
@behnammodi behnammodi force-pushed the keep-classes-when-sheet-and-dynamicRules-not-changed branch from ddd2db0 to 420c62a Compare October 26, 2021 06:34
@behnammodi behnammodi force-pushed the keep-classes-when-sheet-and-dynamicRules-not-changed branch from 640dee0 to 6ed69e7 Compare October 26, 2021 10:35
@behnammodi
Copy link
Member Author

@kof Thanks for review, all of them is done

@behnammodi behnammodi requested a review from kof October 26, 2021 10:54
@behnammodi
Copy link
Member Author

@kof This is ready to merge

@kof
Copy link
Member

kof commented Nov 1, 2021

@behnammodi thanks, will look into it, in the meantime, please test it in your app in production and let me know if you notice anything

@behnammodi
Copy link
Member Author

@behnammodi thanks, will look into it, in the meantime, please test it in your app in production and let me know if you notice anything

Sure, I'll tell you

@behnammodi
Copy link
Member Author

behnammodi commented Nov 1, 2021

@kof I build react-jss then I created a pack and install on our project everything works very well

@behnammodi
Copy link
Member Author

@kof I found a better solution, please see this file packages/react-jss/src/createUseStyles.js changes

@behnammodi
Copy link
Member Author

behnammodi commented Nov 12, 2021

@kof This PR ready to merge and main change is:

-    const classes = sheet && dynamicRules ? getSheetClasses(sheet, dynamicRules) : emptyObject
+    const classes = React.useMemo(
+      () => (sheet && dynamicRules ? getSheetClasses(sheet, dynamicRules) : emptyObject),
+      [sheet, dynamicRules]
+    )

not anymore

@kof
Copy link
Member

kof commented Nov 12, 2021

looks great now, thank you

@kof kof merged commit fc379ea into cssinjs:master Nov 12, 2021
@kof
Copy link
Member

kof commented Dec 8, 2021

released in v10.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants