[RAC] Unmemoized render values passed to useRenderProps()

[nwidynski]

Provide a general summary of the issue here

When rendering a RAC with a memoized render callback, the callback is invoked on each render. This happens regardless of whether the components state or rather its render values have actually changed.

When performing expensive calculations inside a render function, this is especially impactful.

🤔 Expected Behavior?

RAC's should not run a memoized render function twice if the render value hasn't changed. This would allow the developer to make the decision whether or not to memoize the result of a render.

😯 Current Behavior

RAC's invoke their render callbacks even when the render values haven't changed.

💁 Possible Solution

Memoize the values argument passed to useRenderProps in most RAC's.

Example of refactored Button.tsx:

const values = {
  isHovered, 
  isPressed,
  isFocused, 
  isFocusVisible, 
  isDisabled: props.isDisabled || false
};

let renderProps = useRenderProps({
  ...props,
  // eslint-disable-next-line react-hooks/exhaustive-deps
  values: useMemo(() => values, Object.values(values)),
  defaultClassName: 'react-aria-Button'
});

🔦 Context

No response

🖥️ Steps to Reproduce

Example: Codesandbox

1. Choose any RAC utilizing render props.
2. Force a re-render of the parent, while the props of the RAC don't change.
3. Observe another execution of the render function.

Version

latest

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

OSX

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[snowystinger]

Thanks for your thoughts! Unfortunately, I don't think this solves it. You'd have to memo the children of the component as well. https://github.com/adobe/react-spectrum/blob/b940126e4d67a11d28b7c0b098eab23205598b6c/packages/react-aria-components/src/utils.tsx#L95

This discussion is related https://github.com/adobe/react-spectrum/discussions/2569#discussioncomment-1672680

Instead of running expensive operations in render, I suggest using an effect.

[nwidynski]

@snowystinger Hey Robert, thanks for getting back to me.

The scenario with the expensive calculations was meant as an exemplary worst case. I opened this issue because its impossible for the developer to make the choice whether to memoize or not, since the value has a new identity each time.

If this is desired behavior, the useMemo() inside useRenderProps() can be removed since there is no backwards compatibility issue and every RAC constructs its render value unmemoized.

[snowystinger]

Desired might be a strong word. I don't think the memo is hurting anything by being there though it'd probably make more sense to memo some of those values individually and then join them.

I also chased down the state returned by useToggleState isn't memoed, so that'd also create a new useRenderProps value. Honestly, I think this is something the compiler should be good at solving.

I'm going to close this issue, we can reopen if it's causing a problem.