jss icon indicating copy to clipboard operation
jss copied to clipboard

Stylesheet.classes is never reused, even for static styles

Open heswell opened this issue 4 years ago • 4 comments

Expected behavior: I am using react-jss. I use a totally static style declaration object, passed once to createUseStyles, but consumed by many components (using useStyles). I expect all components consuming the generated styles to receive the same classes object instance. In fact a new instance of the classes object is generated for every consuming component (all with identical properties).

Describe the bug: Calls to addDynamicRules always return a truthy value (an empty object literal) even when there are no dynamic rules. This is subsequently tested and, because truthy, new classes object is always created.

call to addDynamicRules always returns object for dynamicRules even when there are no dynamic rules, hence dynamicRules is truthy from now on

from utils/sheets.js

export const addDynamicRules = (sheet: StyleSheet, data: any): ?DynamicRules => {
  const meta = getMeta(sheet)
  // Note meta looks like this {styles: object, dynamicStyles: null }
  if (!meta) {
  // I believe this should also return here if dynamicStyles is null ? 
    return undefined
  }
... continue and return empty object as dynamic rules
}

In the following snippet, dynamicRules is required to be truthy before getSheetClasses is called, but getSheetClasses will only return existing classes without recalculation if dynamivStyles is falsey !

from createUseStyles.js

const classes = sheet && dynamicRules ? getSheetClasses(sheet, dynamicRules) : {}

Codesandbox link: https://codesandbox.io/s/recursing-jepsen-pol0s

Versions (please complete the following information):

  • jss: react-jss: 10.1.1
  • Browser: chrome
  • OS: macOS Feel free to add any additional versions which you may think are relevant to the bug.

Happy to submit a PR if confirmed that this is. a bug and not intended behaviour.

heswell avatar May 26 '20 20:05 heswell

I can see how the code is suboptimal in react-jss because it has to call getSheetClasses where it doesn't need to, but I still haven't understood what use case same reference to classes is solving.

A fix for this can be still accepted as a performance optimization, but only if it also contains a test.

kof avatar May 28 '20 08:05 kof

My use case is precisely the performance optimisation. My real-world example is a complex component that contains many (hundreds / thousands) of nested components, all using the same stylesheet.

I'll prepare a fix/test and see what you think.

heswell avatar May 28 '20 09:05 heswell

I don't know enough of your context, but this is a very unusual scenario, you are either having some very special case or architecting the app in a very "bad" way

kof avatar May 28 '20 09:05 kof

As I said it's a complex component (real-time datagrid). My alternative is to just create the stylesheet once and use my own context to share it

heswell avatar May 28 '20 09:05 heswell