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

react-jss: Performance degradation with dynamic values #1510

Open rtivital opened 3 years ago

rtivital commented 3 years ago

Hi, I'm building components library with react-jss and when my stylesheets got large and complex I've started to experience performance degradation with dynamic values.

Demo

Source code for demo component

Additional example

Video showcase with another component with reproduction instructions

Explanation

I think that this issue is caused by this effect. useStyles accept data object which is used as dependency in useLayoutEffect. If you use styles something like this (similar to docs examples):

useStyles({ size: 'sm', color: 'red' });

Then in createUseStyles useEffect it will resolve in:

useEffectOrLayoutEffect(() => {
  if (sheet && dynamicRules && !isFirstMount.current) {
    updateDynamicRules(data, sheet, dynamicRules);
  }
}, [{ size: 'sm', color: 'red' }]);

Which means that updateDynamicRules function will be called at each render, this is not noticeable with small amount of styles but degrades performance significantly with larger stylesheets.

My solution

I've created a drop in replacement for createUseStyles function that updates props object based on dependencies (it is specific to my project):

function createMemoStyles(styles) {
  const useStyles = createUseStyles(styles);

  return function useMemoStyles(props) {
    const dependencies =
      typeof props === 'object' && props !== null
        ? Object.keys(props)
            .filter((key) => key !== 'theme')
            .map((key) => props[key])
        : [];

    if (typeof props === 'object' && 'theme' in props) {
      dependencies.push(props.theme.colorScheme);
    }

    const stylesProps = useMemo(() => props, dependencies);
    return useStyles(stylesProps);
  };
}

This replacement resolves all performance issues for me.

Possible solution

Calculate effect dependencies based on data in useStyles:

function useStyles(data: any, dependencies: any[] = []) {
  const dependencies =
    typeof data === 'object' && data !== null
      ? Object.keys(data)
          .filter((key) => key !== 'theme')
          .map((key) => data[key])
      : [data];

  useEffectOrLayoutEffect(() => {
    // We only need to update the rules on a subsequent update and not in the first mount
    if (sheet && dynamicRules && !isFirstMount.current) {
      updateDynamicRules(data, sheet, dynamicRules);
    }
  }, [dependencies, ...dependencies]);
}

This will solve issues with primitive values ({ size: 'sm', color: 'red' } from example above) and will allow to subscribe to effect with any extra values. Theme is filtered as it is always an object and will trigger effect on each render.

HenriBeck commented 3 years ago

@rtivital, the code from your example has two issues:

The biggest one is that it only reacts to changes to the values of the keys but not the keys themselves. So while this assumption/statement can be made about your use/library, this isn't feasible for public implementation.

func Component(props) {
  let data = { keyA: 'red' }
  if (props.disabled) {
    data = { keyB: 'red' }
  }

  const styles = useStyles(data)
}

While this code looks weird and is not great by any means, it still needs to be supported by us.
The code also only works for "flat" objects; e.g., if you have an object as the value in your data object, you would still have the same issue.


Secondly, the useStyles hook behaves the same way as any other React hook as the dependencies are not checked for deep equality but rather reference equality.
In my opinion, the proper usage in such a case should be to wrap the data object with a React.useMemo() on the caller side rather than the library.

You could then also think about writing a small utility hook that does this for you based on the dynamic object, e.g.:

const useMemoObject(obj) => React.useMemo(() => obj, [...Object.entries(obj)])
rtivital commented 3 years ago

Thanks for quick response

the proper usage in such a case should be to wrap the data object with a React.useMemo() on the caller side rather than the library.

yep, that's true, useMemo is something like what I've implemented with some project specific dependencies.

I was trying to come up with better solution but seems like there is not any good one as data may include objects and I guess there can be many other edge cases of which I'm not aware about.

Anyway, memoization solved my issue, just wanted to share possible solution, since it won't work, I think it's a good idea to add this to docs to avoid these kind of confusions. I can add it and create a PR if you want.

kof commented 3 years ago

This is indeed gonna be quite a bit slower if that component updates frequently. I wonder if we could implement some very simple and efficient shallow comparison here in useStyles, because people who use useStyles directly are not gonna implement memoization themselves and asking them to memoize the data is big ask.

We could run the shallow comparison inside updateDynamicRules, after referential equality already failed

    useEffectOrLayoutEffect(
      () => {
        // We only need to update the rules on a subsequent update and not in the first mount
        if (sheet && dynamicRules && !isFirstMount.current) {
          updateDynamicRules(data, sheet, dynamicRules)
        }
      },
      [data]
    )
pybuche commented 3 years ago

@kof Any updates on this? We have the exact same problem, making our UI impossible to use... The easiest way to see it is by analyzing our Text component, that handle size (which can be responsive), color & weight

type TextProps = {
  color: string;
  size: "xs" | "sm" | "md";
  weight: 400 | 600 | 800;
};

// The createUseStyles hook
const useTextStyles = createUseStyles({
  text: ({ color = "grey900", size, weight }: TextProps) => {
    return {
      fontWeight: weight,
      color: colors[color],
      ...responsiveStyle(size, sizes.text),
    };
  },
});

// Usage in Text
const Text = (props: TextProps) => {
    const styles = createUseStyles(props);
    return <span className={styles.text}>{props.children}</span>;
}

// Responsive style helper
export function isResponsiveObject<T>(
  value: T | Responsive<T>
): value is Responsive<T> {
  return _.isObject(value);
}

/**
 * @param propValue: The current value of your property. Can be a single value ("size24") or a responsive object, representing { [breakpointValue]: propertyValue }, (ex: { xs: "size24", md: "size36" })
 * @param availableStyles: The list of CSS properties for a given `propValue` ({ size24: { width: 24}, size36: { width: 36 } })
 */
export const responsiveStyle = <T extends string>(
  propValue: T | Responsive<T>,
  availableStyles: { [key in T]: CSSProperties }
) => {
  // If `propValue` is a single value, find the value directly in the provided list of available styles
  if (!isResponsiveObject(propValue)) return availableStyles[propValue];

  // If `propValue` is a responsive object, map every breakpoint with there associated style
  // Ex: propValue = { xs: "size24", md: "size36" }, availableStyles = { size24: { width: 24}, size36: { width: 36 } }
  // responsiveStyles = { xs: { width: 24}, md: { width: 36 } }
  const responsiveStyles = _.mapValues(
    propValue,
    (responsiveValue) => availableStyles[responsiveValue]
  );

  // Finally, replace breakpoint name by media queries keys
  // Ex: responsiveStyles = { xs: { width: 24}, md: { width: 36 } }
  // Result = { "@mediaForXS": { width: 24}, "@mediaForMd": { width: 36 } }
  const responsiveStyleProps = _.mapKeys(
    responsiveStyles,
    (style, breakpoint) => responsiveUp(breakpoint as Breakpoint)
  );

  // Fallback on last minimal value so there is no unhandled case
  // Ex: with the example above
  // Result: { "@mediaForXS": { width: 24}, "@mediaForMd": { width: 36 }, width: 24 }
  const minimalBreakpoint = _.minBy(
    Object.keys(propValue),
    (breakpoint) => breakpoints[breakpoint]
  );
  return {
    ...responsiveStyleProps,
    ...responsiveStyles[minimalBreakpoint],
  };
};

And here's a Performance analysis when rendering hundreds of Text components. We clearly see updateDynamicRules being called a lot and lasting quite a long time (~150/200) Screenshot 2021-08-05 at 11 19 46

sscaff1 commented 2 years ago

Any updates on this issue? Or any recommended approaches?

kjrgreen commented 1 year ago

The issue I was experiencing was poor performance due to dynamic values changing multiple times per second, namely in reaction to scrolling. I side-stepped using dynamic values in this manner, using css variables:

const useStyles = createUseStyles(() => ({
    movingElement: {
        backgroundPosition: "var(--scroll)",
    },
}));

function MovingElement() {
    const classes = useStyles();
    const rapidlyChangingValue = 0; // Just pretend that this changes rapidly
    return <div className={classes.movingElement} style={{
        '--scroll': `center ${rapidlyChangingValue}px`,
    }}/>;
}

Typescript might scream at you for using variable names in the inline styles. Here's a resource for how to handle that.