Optimization of the optimization

[garronej]

Optimization of the optimization

[garronej]

Hi @olee,
I'm off today. I think your objections come from the fact that you though I was less carefull when implementing this that I actually was.
Please take a few moments to review what the code actully does. If you still think there's a problem after that I will update the code swiftly.
Best

[olee]

You are right and wrong a the same time 😄

I checked it in detail once more and noticed you actually only generate "thumbprints" of the passed object for plain objects where the values in the object are only primitives which was a really good consideration and would indeed ensure that the function cannot crash afaik.

However, for ensuring performance in all cases and to be in line with React's rules of hooks, I still think the option I proposed would be more beneficial.

• Using useMemo on the object allows for any structure of object to be optimized
• Json serialization still is a considerable performance impact - especially if you have objects which have many properties
• Your code would produce inconsistent behavior where some types of passed props would be memoized (simple props only without any children, styles or callbacks passed), but most other cases where regular props objects are passed which contain children, arrays, styles or callbacks would not be memoized

Trying to use the option I proposed with using useMemo would be even be destroyed by your code in some cases:

// This code would work with the current memoization, but even though the very same object is passed, the json serialization and all the other checks would still run
const { classes } = useStyles(
  useMemo(() => ({
    color: props.color,
  }), [props.color])
);

// This code would always recompute classes, even if the object is the same
const { classes } = useStyles(
  useMemo(() => ({
    color: props.color,
    componetOptions: {
        bold: props.bold,
        italic: false,
    }
  }), [props.color, props.bold])
);

This is why I think it is a better option to stay with "simple is best" here and leave the details to the developer actually implementing the styles 👍

[garronej]

function cannot crash

It can. Getters can throw exceptions.

Using useMemo on the object allows for any structure of object to be optimized

Big no, I will never recommend implementing this approach. It obfuscates the code and requires additional maintenance for the sake of a very hypothetical performance gain.

Json serialization still is a considerable performance impact - especially if you have objects which have many properties

There is no JSON serialization going on. I used JSON.stringify (not anymore) only so that { "foo": "3" } and { "foo": 3 } wouldn't have the same fingerprint.

Your code would produce inconsistent behavior

No, unpredictable performances.

but most other cases where regular props objects are passed

Passing the props object directly to useStyles is not an approach I recommend. I know many peoples do, though. Even I used to because it was suggested in the Material-ui documentation. Well, it's no big deal, if there is a callback in the props the styles are going to be recomputed.

This code would always recompute classes, even if the object is the same

No, it won't.

This is why I think it is a better option to stay with "simple is best"

In the early days of tss-react I had [a very sophisticated optimization algorithm] (https://github.com/garronej/tss-react/blob/3587bd6f5f088e794787fb03d0f2150521cc2934/src/createUseClassNamesFactory_optimized.ts) in place. It was relying on Proxy to check what properties were being accessed in useStyles and prevent subsequent re-computation whenever possible. I ditch this for three reasons:

• It required to have memoizee as a dependency.
• Material-ui's team was starting to show interest for the lib. I wanted the code to be understandable.
• There is already memoization going on with @emotion/react. I was unsure what I was doing had an actual significant impact on performances.

Now, all that said, it is very much possible that the optimization currently in place is detrimental for the performances. I didn't do any complexity analysis nor benchmark.
What I can say for sure, though, is that if running this is slower than actually computing the classes then we are optimizing something that doesn't need to be.

Thank you for your input. I'll close this now but you are more than welcome to help me find a solution for withStyles, it's the only blocker before we can put your cool API for nested selectors in production. I have opened a PR