[Select][base] Prevent unnecessary rerendering of Select options #35946

michaldudak · 2023-01-25T15:41:53Z

This PR solves a performance issue with SelectUnstyled and dependent components (Joy's Select).

The root of the problem was that every mouseover event fired on an option caused rerendering of all options. The event correctly dispatches the optionHover action in the listbox reducer, but the surrounding logic caused some functions to be recreated, which caused the SelectUnstyledContext to be updated. This, in turn, caused all the options to detect the changes and rerender themselves.

A couple of optimizations were made:

Functions that don't need to be recreated every render were wrapped in useCallback
The SelectUnstyledContext value does not change when the selected or highlighted items change. Instead, a new way of notifying the options about changes was introduced. The options subscribe to updates in the select, and when an update event is fired, they can decide if the change concerns them and rerender accordingly.

The introduction of the lightweight event bus was necessary to update only the options that were changed. The Material UI implementation doesn't have this problem, as it uses cloneElement, so that the Select can set props directly on its MenuItems. This, however, can cause issues when grouping is used and prevents from using non-MenuItem components (or even Fragments) as Select's children. The discussion about removing cloneElement usage took place in #14943.

With these changes, the Option component has more to do. To prevent developers from having to reimplement this logic in their own design systems, the useOption hook was added.

Additionally, I refactored the code in a few places.

Breaking change

This PR changes what the useSelect and useMenu hooks return.

useSelect

The getOptionProps and getOptionState functions that were returned by useSelect are now members of the contextValue object returned by the hook.

-const { getOptionProps, getOptionState, value } = useSelect({ // ...
+const { contextValue, value } = useSelect({ // ...
+const { getOptionProps, getOptionState } = contextValue;

The whole contextValue object is intended to be passed into the SelectUnstyledContext.Provider that wraps all options.

useMenu

The getItemProps, getItemState, registerItem, and unregisterItem functions that were returned by useMenu are now members of the contextValue object returned by the hook.

-const { getOptionProps, getOptionState, registerItem, unregisterItem } = useMenu({ // ...
+const { contextValue } = useMenu({ // ...
+const { getOptionProps, getOptionState, registerItem, unregisterItem } = contextValue;

The whole contextValue object is intended to be passed into the MenuUnstyledContext.Provider that wraps all items.

Before: https://codesandbox.io/s/practical-oskar-ibmwvn?file=/demo.js
After: https://codesandbox.io/s/gifted-antonelli-srtk0u?file=/demo.js

This fixes #34112.

+ fix all the TS errors

mui-bot · 2023-01-25T15:46:01Z

Netlify deploy preview

https://deploy-preview-35946--material-ui.netlify.app/

@material-ui/unstyled: parsed: +2.25% , gzip: +2.49%
@mui/joy: parsed: +0.63% , gzip: +0.77%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against bd46081

…performance

oliviertassinari · 2023-02-08T10:02:18Z

The root of the problem was that every mouseover event fired

Did we consider mouseenter? This event fires less often than the mouseover event. I don't recall why I didn't use mouseenter more in the past.

Functions that don't need to be recreated every render were wrapped in useCallback

Did you measure differences with this? I'm asking because I believe the default outcome with them is more CPU cycles (it takes more time to compare than to recreate the functions). However, it does make a difference when it prunes large branches of work (like not rendering 10 children).

After: https://codesandbox.io/s/gifted-antonelli-srtk0u?file=/demo.js

~~I think that the problem is present in this codesandbox, the hover effect lags +100ms behind.~~ It seems fix, there is some JavaScript running on mouseover but it's a lot less work than before, and seems to be coming from React itself and not MUI Base 👍

michaldudak · 2023-02-08T10:17:27Z

Did we consider mouseenter? This event fires less often than the mouseover event. I don't recall why I didn't use mouseenter more in the past.

No, but in this case, I don't think it would make a difference. The problem was rerendering all options when hover moved from one to another.

the default outcome with them is more CPU cycles (it takes more time to compare than to recreate the functions)

Do you have any resources on this? I wonder how can comparing two references be slower than recreating a function. Anyway, in this case, recreating functions that are placed in a context causes a rerender of all listeners of the context (all options), which is definitely slower.

I think that the problem is present in this codesandbox, the hover effect lags +100ms behind

It could be perceived as that because of the transition of the background color. I disabled it so the difference is more noticable.

oliviertassinari · 2023-02-08T10:54:29Z

The problem was rerendering all options when hover moved from one to another.

@michaldudak Ok, it might have been because of bugs with SVGs or some element that wrongly trigger the mousenter event.

One thing I struggle to understand is the use case: why does the select need to know the hovered option? The Select in Material UI doesn't listen to it, the Autocomplete does but I think that it's a mistake I did per #23916. I shouldn't have mixed the hover and keyboard focus states, it breaks the UX and creates bugs like #20602.

Do you have any resources on this? I wonder how can comparing two references be slower than recreating a function.

The why: https://kentcdodds.com/blog/usememo-and-usecallback. It boils down to: useCallback is extra CPU work, the runtime still needs to recreate the function at each render.

It could be perceived as that because of the transition of the background color. I disabled it so the difference is more noticable.

Oh the transition, yes! @siriwatknp How about we remove all these transitions on hover (here and in other components when the delay to feedback is important)? It doesn't seem to help and doing a benchmark with other libraries, I can't find selects that have it that still feel great to use. It feels unresponsive (slow).

Off-topic. It looks like we have a bug with the scroll positioning when opening the select:

Joy UI: https://codesandbox.io/s/frosty-diffie-h5cobm?file=/demo.js wrong position
MUI Base: https://codesandbox.io/s/beautiful-haze-6p5o5w?file=/demo.tsx wrong position
Material UI: https://codesandbox.io/s/pedantic-danny-euuyd2?file=/demo.js right position (super slow though)

It seems to be that the select is going beyond the viewport, we likely miss a max height of 100vh.

michaldudak · 2023-02-08T11:07:38Z

One thing I struggle to understand is the use case: why does the select need to know the hovered option?

This was a tough decision to make. Native <select> elements seem to use both mouse and keyboard to highlight an item. I am aware of the problems we have with Autocomplete. That's why I decided to make it configurable, so developers can choose what they like most. The code that runs on option hover calls a reducer with the optionHover action. By default, the built-in reducer just ignores the action, so nothing gets rerendered. However, a developer may choose to provide their own reducer that can highlight an option.

Oh the transition, yes! @siriwatknp How about we remove these transitions on hover (here and in other components when the delay to feedback is important)?

We could leave the "out" transition, but yes, I'd remove the "in" one.

Off-topic. It looks like we have a bug with the scroll positioning when opening the select

Thanks for pointing this out. The Select demo needs a max-height. The Joy Select may as well.

the runtime still needs to recreate the function at each render.

But thanks to this, we can prevent thousands of renders. I agree that, in some cases, the performance gain is not noticeable. Here, it definitely is. I'm not using useCallback here to minimize the number of function allocations but to prevent unnecessary rerenders.

oliviertassinari · 2023-02-08T11:22:57Z

By default, the built-in reducer just ignores the action, so nothing gets rerendered.

@michaldudak Ah ok, looks great 👍

We could leave the "out" transition, but yes, I'd remove the "in" one.

To experiment, I would expect no transition to feel better for hover. However, an "out" transition and no "in" sounds like a great idea to experiment with for the autocomplete, select, and date picker popups! Right now the autocomplete popup has no transitions, which I think feels great, but an out transition could feel even better. The date picker has an "in" transition which makes it feel too slow to be great (point 12 of mui/mui-x#7440): cc @gerdadesign.

siriwatknp · 2023-02-09T07:47:41Z

Oh the transition, yes! @siriwatknp How about we remove all these transitions on hover (here and in other components when the delay to feedback is important)?

Agree, #35952 will take care of that.

siriwatknp · 2023-02-09T08:01:16Z

@michaldudak Maybe this is off-topic but I wonder if the value prop should be required for the Option component?

Without the value prop, it looks like a bug https://codesandbox.io/s/joy-cra-typescript-forked-h6ybih?file=/src/App.tsx

siriwatknp · 2023-02-09T08:13:31Z

From Joy's perspective, this is a huge improvement! 👏 Great job @michaldudak.

mnajdova

Small nits here and there, apart from that I can't sport anything else. Great work @michaldudak!

mnajdova · 2023-02-10T12:37:23Z

docs/data/base/components/select/UseSelect.tsx

+  const { children, value, className } = props;
+  const { getRootProps, highlighted } = useOption({
+    value,
+    disabled: false,


I would do either:

Not add this at all

Suggested change

disabled: false,

Use the value from the props (we can initialize it above to false):

Suggested change

disabled: false,

disabled,

This is closer to what developers would actually need.

mnajdova · 2023-02-10T12:38:41Z

packages/api-docs-builder/buildApiUtils.ts

@@ -47,6 +47,7 @@ const parseFile = (filename: string) => {
    shouldSkip:
      filename.indexOf('internal') !== -1 ||
      !!src.match(/@ignore - internal component\./) ||
+      !!src.match(/@ignore - internal hook\./) ||


mnajdova · 2023-02-10T12:41:52Z

packages/mui-base/src/ListboxUnstyled/useControllableReducer.test.tsx

@@ -53,25 +54,28 @@ describe('useControllableReducer', () => {
      });

      const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b', event: null };
+      const props: UseListboxPropsWithDefaults<string> = {


Can we extract this above and define it only once?

I did this to make the tests more self-sufficient (it's easier to read when you have everything nearby). I don't think repetition is a problem is tests.

mnajdova · 2023-02-10T12:48:22Z

packages/mui-base/src/ListboxUnstyled/useListbox.ts

+      const optionState = getOptionState(option);
+      const index = options.findIndex((opt) => optionComparer(opt, option));


Suggested change

const optionState = getOptionState(option);

const index = options.findIndex((opt) => optionComparer(opt, option));

const optionState = getOptionState(option);

const { index } = optionState;

nit: we already are finding the index in getOptionState, we could return it as part of the state. Not sure if it's worth the change

Good point! This could make it even faster a bit.

michaldudak · 2023-02-13T13:18:23Z

Maybe this is off-topic but I wonder if the value prop should be required for the Option component?

I'll analyze it and create a separate PR.

…performance

smellai · 2023-02-28T17:08:01Z

I think some more breaking change was introduced in this release. Without changing the options array, the onChange event handler now gets object instead of string, and also there's a type mismatch there. Also highlighted state from optionState doesn't seem to be set correctly anymore.

https://codesandbox.io/s/heuristic-hypatia-brfyof?file=/demo.tsx

michaldudak · 2023-03-01T13:48:22Z

@smellai Could you please open a new issue so we can track this problem?

enricoros · 2023-05-11T02:30:54Z

Hi, starting with Joy 5.0.0-alpha.76 (.75 is working fine, .76+ are broken), Select > Options > text changes will not update the Select component.

The following code creates the options:

  <Select ...>
    {items.map((item: {key: string, label: string}, idx: number) => (
      <Option key={'opt-' + idx} value={item.key}>
        {symbol ? '-' : ''} {item.label}
      </Option>
    ))}
  </Select>

When toggling symbol:boolean, the Select will need a click to show the updated options, including the update in the "Select" itself.

The current workaround is to fix the version of Joy to alpha-75.

siriwatknp · 2023-05-11T03:46:17Z

Hi, starting with Joy 5.0.0-alpha.76 (.75 is working fine, .76+ are broken), Select > Options > text changes will not update the Select component.

The following code creates the options:
  <Select ...>
    {items.map((item: {key: string, label: string}, idx: number) => (
      <Option key={'opt-' + idx} value={item.key}>
        {symbol ? '-' : ''} {item.label}
      </Option>
    ))}
  </Select>
When toggling symbol:boolean, the Select will need a click to show the updated options, including the update in the "Select" itself.

The current workaround is to fix the version of Joy to alpha-75.

Thanks for reporting the issue! Can you open a new issue?

michaldudak added 22 commits January 16, 2023 17:01

[Select][base] Reduce the number of renders

689d18f

Further optimization

7d8bf0e

Create helper hooks

6d04c17

Decouple useTextNavigation from listbox

4a7fb2a

Remove debug statements

c0462ac

Augment dispatch with props

f7ff578

Simplify

3608e2f

Fix the initial highlight issue

a79de75

Memoize the internal reducer

101c9a9

Fix Joy tests

904173f

Make Select's listbox deal with plain values instead of SelectOptions

d2c2bc2

+ fix all the TS errors

Fix lint error

24376b8

Fix all the tests

c482c86

Remove todos

cff80f0

Lint

28b9824

Create the useSelectChangeNotifiers hook

87b67d5

Removed double source of truth of the option selected state

31cdaf9

Fix the listbox' uncontrolled mode

d252ec9

Cleaning up

b5c685a

Create eventBus

706c25e

Fix the remaining errors

bb81920

Add new type

05ccc31

michaldudak added component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy bug 🐛 Something doesn't work and removed package: joy-ui Specific to @mui/joy labels Jan 25, 2023

michaldudak requested review from siriwatknp and mnajdova January 25, 2023 15:57

michaldudak added 3 commits February 8, 2023 09:30

Merge remote-tracking branch 'upstream/master' into iss/34112-select-…

5abb5e4

…performance

docs:typescript:formatted

880bdaf

Do not document internal hooks

b4f216f

oliviertassinari mentioned this pull request Feb 8, 2023

[pickers] UX design review v6.0.0-alpha.14 mui/mui-x#7440

Open

Wrap Joy's handleOpenChange in useCallback

9e2cfcf

siriwatknp mentioned this pull request Feb 9, 2023

[Joy] Select popup should have max-height #36115

Closed

2 tasks

mnajdova approved these changes Feb 10, 2023

View reviewed changes

Address PR remarks

47b739d

michaldudak added the breaking change label Feb 13, 2023

michaldudak added 3 commits February 13, 2023 14:27

Add index to useOption return type

e5851c7

Merge remote-tracking branch 'upstream/master' into iss/34112-select-…

b771b95

…performance

Docs

bd46081

michaldudak merged commit 1f9d2b1 into mui:master Feb 13, 2023

michaldudak deleted the iss/34112-select-performance branch February 13, 2023 14:23

michaldudak mentioned this pull request Feb 14, 2023

[Select][base] Update the generated docs #36183

Merged

smellai mentioned this pull request Mar 1, 2023

[Base][useSelect] onChange event handler returns an object #36386

Closed

2 tasks

enricoros mentioned this pull request May 11, 2023

[Joy][Select] Does not re-render Options when changing text #37235

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Select][base] Prevent unnecessary rerendering of Select options #35946

[Select][base] Prevent unnecessary rerendering of Select options #35946

michaldudak commented Jan 25, 2023 •

edited

mui-bot commented Jan 25, 2023 •

edited

oliviertassinari commented Feb 8, 2023 •

edited

michaldudak commented Feb 8, 2023 •

edited

oliviertassinari commented Feb 8, 2023 •

edited

michaldudak commented Feb 8, 2023 •

edited

oliviertassinari commented Feb 8, 2023 •

edited

siriwatknp commented Feb 9, 2023

siriwatknp commented Feb 9, 2023

siriwatknp commented Feb 9, 2023

mnajdova left a comment

mnajdova Feb 10, 2023

mnajdova Feb 10, 2023

mnajdova Feb 10, 2023

michaldudak Feb 13, 2023

mnajdova Feb 10, 2023

michaldudak Feb 13, 2023

michaldudak commented Feb 13, 2023

smellai commented Feb 28, 2023

michaldudak commented Mar 1, 2023

enricoros commented May 11, 2023 •

edited

siriwatknp commented May 11, 2023

		const optionState = getOptionState(option);
		const index = options.findIndex((opt) => optionComparer(opt, option));

[Select][base] Prevent unnecessary rerendering of Select options #35946

[Select][base] Prevent unnecessary rerendering of Select options #35946

Conversation

michaldudak commented Jan 25, 2023 • edited

Breaking change

useSelect

useMenu

mui-bot commented Jan 25, 2023 • edited

Netlify deploy preview

Bundle size report

oliviertassinari commented Feb 8, 2023 • edited

michaldudak commented Feb 8, 2023 • edited

oliviertassinari commented Feb 8, 2023 • edited

michaldudak commented Feb 8, 2023 • edited

oliviertassinari commented Feb 8, 2023 • edited

siriwatknp commented Feb 9, 2023

siriwatknp commented Feb 9, 2023

siriwatknp commented Feb 9, 2023

mnajdova left a comment

Choose a reason for hiding this comment

mnajdova Feb 10, 2023

Choose a reason for hiding this comment

mnajdova Feb 10, 2023

Choose a reason for hiding this comment

mnajdova Feb 10, 2023

Choose a reason for hiding this comment

michaldudak Feb 13, 2023

Choose a reason for hiding this comment

mnajdova Feb 10, 2023

Choose a reason for hiding this comment

michaldudak Feb 13, 2023

Choose a reason for hiding this comment

michaldudak commented Feb 13, 2023

smellai commented Feb 28, 2023

michaldudak commented Mar 1, 2023

enricoros commented May 11, 2023 • edited

siriwatknp commented May 11, 2023

michaldudak commented Jan 25, 2023 •

edited

mui-bot commented Jan 25, 2023 •

edited

oliviertassinari commented Feb 8, 2023 •

edited

michaldudak commented Feb 8, 2023 •

edited

oliviertassinari commented Feb 8, 2023 •

edited

michaldudak commented Feb 8, 2023 •

edited

oliviertassinari commented Feb 8, 2023 •

edited

enricoros commented May 11, 2023 •

edited