cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.08k stars 400 forks source link

Stylesheet.classes is never reused, even for static styles #1347

Open heswell opened 4 years ago

heswell commented 4 years ago

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):

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

kof commented 4 years ago

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.

heswell commented 4 years ago

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.

kof commented 4 years ago

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

heswell commented 4 years ago

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