[material-ui] Performance: lazy Ripple #41061
Conversation
Netlify deploy previewhttps://deploy-preview-41061--material-ui.netlify.app/ SpeedDial: parsed: +0.88% , gzip: +0.88% Bundle size reportDetails of bundle changes (Toolpad) |
oliviertassinari
added
the
component: button
|
A side note, the TouchRipple seems to be somehow involved as a "scroll rect" in the Layers: Screen.Recording.2024-02-11.at.20.51.32.movI assume we don't need to care about this, but funny. |
|
This looks good to me, @romgrk 🤗 I think we should move forward with it Regarding other places this change has to be implemented, this is the only one I can think of: https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/ButtonBase/ButtonBase.tsx. If I recall correctly, the ripple implementation is slightly different there, as it was updated. This lazy mounting implementation looks useful 🙌🏼 I wonder if we have something similar somewhere else, either in Material UI or other products 🤔 CC @mnajdova in case you can spot anything I can't regarding the ripple implementation |
|
I've been trying to find time for this but I have more pressing optimizations to merge on the datagrid. The implementation is complete but the change broke a lot of tests which were all expecting the ripple to be mounted. If someone has interest in this PR feel free to look into those, otherwise I'll come back to it once I have more time. |
github-actions
bot
added
the
PR: out-of-date
|
In Toolpad we've been doing conditional rendering during SSR/hydration without introducing secondary renders when the component mounts client-side by leveraging |
github-actions
bot
removed
the
PR: out-of-date
| if (process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| React.useEffect(() => { | ||
| if (enableTouchRipple && !rippleRef.current) { | ||
| console.error( | ||
| [ | ||
| 'MUI: The `component` prop provided to ButtonBase is invalid.', | ||
| 'Please make sure the children prop is rendered in this custom component.', | ||
| ].join('\n'), | ||
| ); | ||
| } | ||
| }, [enableTouchRipple]); | ||
| } |
There was a problem hiding this comment.
As the ripple is now lazy, using it for this devmode check doesn't make sense anymore. If someone has an idea for an alternative I can restore this warning.
There was a problem hiding this comment.
The history: #20401. I guess taking the initial issue reproduction, and adding the warning where the code crash would do it?
There was a problem hiding this comment.
The initial issue would not happen anymore. With the previous implementation, not rendering the ripple in a custom button component would cause a crash when calling API methods because the ripple ref would be null. With the current implementation, the ripple API calls only go through once the ripple is mounted and its ref is not null; not rendering/mounting the ripple would simply create a promise that never resolves. So the check can't be used anymore, and I don't see an easy way to add back this warning.
| fireEvent.click(menuitem); | ||
|
|
||
| await act(async () => { | ||
| fireEvent.click(menuitem); | ||
| }); |
There was a problem hiding this comment.
The tests are now passing except in jsdom, where 77 tests are failing, and it looks like most or all of them are the same case:
Expected test not to call console.error() but instead received 1 calls.
If the warning is expected, test for it explicitly by using the toErrorDev() matcher.
Test location:
/home/romgrk/src/material-ui/packages/mui-material/test/integration/MenuList.test.js
console.error message #1:
Warning: An update to ForwardRef(TouchRipple) inside a test was not wrapped in act(...).
Because the ripple is lazy, it now runs inside a promise (microtask) handler:
class LazyRipple {
start(...args) {
this.mount().then(() => this.ref.start(...args))
// ^ this runs async
}
}To ensure the state update (the ripple being updated) runs inside act(), we must refactor tests like this:
// before
fireEvent.click(menuitem)
// after
await act(async () => {
fireEvent.click(menuitem);
});Before I go replace the 77 instances of this issue, I was wondering if someone has an idea how to avoid this? I find it strange that it only happens in jsdom, I would expect it to happen in any environment.
There was a problem hiding this comment.
act is still somewhat mysterious to me. But I usually tend to avoid it altogether by reasoning which assertions will need to have effects run, or will asynchronously update. Wrapping these assertion with a waitFor makes the test more understandable to me:
diff --git a/packages/mui-material/test/integration/MenuList.test.js b/packages/mui-material/test/integration/MenuList.test.js
index e8f2cc066e..bf3202e25e 100644
--- a/packages/mui-material/test/integration/MenuList.test.js
+++ b/packages/mui-material/test/integration/MenuList.test.js
@@ -7,6 +7,7 @@ import {
fireEvent,
screen,
programmaticFocusTriggersFocusVisible,
+ waitFor,
} from '@mui/internal-test-utils';
import MenuList from '@mui/material/MenuList';
import MenuItem from '@mui/material/MenuItem';
@@ -493,17 +494,13 @@ describe('<MenuList> integration', () => {
);
const menuitem = getByText('Arizona');
- // user click
- act(() => {
- fireEvent.mouseDown(menuitem);
- menuitem.focus();
- });
- await act(async () => {
- fireEvent.click(menuitem);
- });
+ fireEvent.mouseDown(menuitem);
+ menuitem.focus();
+ fireEvent.click(menuitem);
+
+ await waitFor(() => expect(menuitem).toHaveFocus());
- expect(menuitem).toHaveFocus();
if (programmaticFocusTriggersFocusVisible()) {
expect(menuitem).to.have.class('focus-visible');
} else {And this passes now. I'd also have expected this warning to happen in any env. I'm assuming it's related to how jsdom dom events implementation interact with the js event loop/microtask queue.
There was a problem hiding this comment.
🤔 Another way is to add await act(...) around the menu.focus(). fireEvent already automatically wraps with act internally. As for why fireEvent.focus(menuitem); doesn't work isn't fully clear to me.
diff --git a/packages/mui-material/test/integration/MenuList.test.js b/packages/mui-material/test/integration/MenuList.test.js
index e8f2cc066e..1135ca9ccb 100644
--- a/packages/mui-material/test/integration/MenuList.test.js
+++ b/packages/mui-material/test/integration/MenuList.test.js
@@ -494,14 +494,10 @@ describe('<MenuList> integration', () => {
const menuitem = getByText('Arizona');
// user click
- act(() => {
- fireEvent.mouseDown(menuitem);
- menuitem.focus();
- });
- await act(async () => {
- fireEvent.click(menuitem);
- });
+ fireEvent.mouseDown(menuitem);
+ await act(() => menuitem.focus());
+ fireEvent.click(menuitem);
expect(menuitem).toHaveFocus();
if (programmaticFocusTriggersFocusVisible()) {Source:
There was a problem hiding this comment.
For the remaining failing tests, the tests aren't about the ripple so act isn't even useful; the warning about using act is really the only thing that make those tests fail. waitFor would indirectly solve the problem, not because it's waiting for anything useful but only because awaiting for it gives enough time for the next microtask to run (where the ripple callback is), but something like await Promise.resolve() or await new Promise(r => setTimeout(r, 0)) would also be enough.
IIUC, fireEvent.focus(element) only dispatches a focus event at the element, while element.focus() actually focuses the element (sets the document.activeElement, etc) and dispatches a focus event though the DOM's logic.
There was a problem hiding this comment.
actis still somewhat mysterious to me
It's implemented here and it ties a lot into react internals to make things synchronous.
There was a problem hiding this comment.
Is the pattern wrong or is the eslint plugin wrong?
There was a problem hiding this comment.
I don't know. Testing Lib helpers don't need to be wrapped in act, so I'd say the pattern is wrong on paper. I normally defer to waitFor for async operations, but I'm not familiar with the lazy ripple implementation and why it needs await act(async () => {.
There was a problem hiding this comment.
The pattern is wrong redundant, following seems to work for me:
remove all act around fireEvent. Change the act around button.focus() to await act(async () => {
if (typeof Touch !== 'undefined') {
const touch = new Touch({ identifier: 0, target: button, clientX: 0, clientY: 0 });
- await act(async () => fireEvent.touchStart(button, { touches: [touch] }));
+ fireEvent.touchStart(button, { touches: [touch] });
expect(onTouchStart.callCount).to.equal(1);
- await act(async () => fireEvent.touchEnd(button, { touches: [touch] }));
+ fireEvent.touchEnd(button, { touches: [touch] });
expect(onTouchEnd.callCount).to.equal(1);
}
if (canFireDragEvents) {
- await act(async () => fireEvent.dragEnd(button));
+ fireEvent.dragEnd(button);
expect(onDragEnd.callCount).to.equal(1);
}
- await act(async () => fireEvent.mouseDown(button));
+ fireEvent.mouseDown(button);
expect(onMouseDown.callCount).to.equal(1);
- await act(async () => fireEvent.mouseUp(button));
+ fireEvent.mouseUp(button);
expect(onMouseUp.callCount).to.equal(1);
- await act(async () => fireEvent.contextMenu(button));
+ fireEvent.contextMenu(button);
expect(onContextMenu.callCount).to.equal(1);
await user.click(button);
expect(onClick.callCount).to.equal(1);
- act(() => {
+ await act(async () => {
button.focus();
});
expect(onFocus.callCount).to.equal(1);
- await act(async () => fireEvent.keyDown(button));
+ fireEvent.keyDown(button);
expect(onKeyDown.callCount).to.equal(1);
- await act(async () => fireEvent.keyUp(button));
+ fireEvent.keyUp(button);
expect(onKeyUp.callCount).to.equal(1);
- act(() => {
+ await act(async () => {
button.blur();
});
expect(onBlur.callCount).to.equal(1);
- await act(async () => fireEvent.mouseLeave(button));
+ fireEvent.mouseLeave(button);
expect(onMouseLeave.callCount).to.equal(1);
});
});I believe this should be compliant with the eslint plugin.
Personally I wouldn't mind if istead we created a const flushUi = () => act(async () => {}) utility and called it after each button.focus() call with await flushUi(). I find that so much easier to reason about. Testing library does not recommend it though. IMO, the most maintainable thing to do is to forget about act altogether and test with waitFor. act is just a leaky abstraction.
There was a problem hiding this comment.
I don't know. Testing Lib helpers don't need to be wrapped in
act, so I'd say the pattern is wrong on paper. I normally defer towaitForfor async operations, but I'm not familiar with the lazy ripple implementation and why it needsawait act(async () => {.
The top-level comment of this thread should explain why the wrapper is needed. The events dispatched by fireEvent trigger asynchronous React state updates (the lazy ripple being mounted), and React will log a warning that any state update must be wrapped in act if the state update happens asynchronously without the wrapper, because it's indeed not in an act context.
Anything that triggers a state update needs to be in act, be it DOM operations like button.focus() or testing-library operations (which call the DOM dispatchEvent underneath).
| <ButtonGroup disableRipple> | ||
| <Button TouchRippleProps={{ classes: { root: 'touchRipple' } }}>Hello World</Button> | ||
| </ButtonGroup>, | ||
| ); | ||
| await ripple.startTouch(getByRole('button')); |
There was a problem hiding this comment.
We prefer not to use any of the return values of render( in the codebase. This is because there is never more than one component mounted, so we can simplify the DX.
| await ripple.startTouch(getByRole('button')); | |
| await ripple.startTouch(screen.getByRole('button')); |
There was a problem hiding this comment.
Their official linter actually catches this. Integrating it is on the code infra board, but this rule is violated all over the place.
There was a problem hiding this comment.
Yeah I noticed both methods were used so I picked the most convenient one on the spot. I'll switch to screen for the changes in this PR.
There was a problem hiding this comment.
The rest of this file is already using getByRole from render a lot, I think I'd prefer to keep the file locally consistent.
There was a problem hiding this comment.
@Janpot Oh, interesting, I forgot testing-library had an eslint plugin. 👍 to move to screen. everywhere in the codebase.
I have promoted your code-infra draft issue, to an open issue: mui/mui-public#173.
There was a problem hiding this comment.
Perfect timing! I didn't know integrating the ESLint plugin was on code-infra's radar already:
| if (process.env.NODE_ENV !== 'production') { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| React.useEffect(() => { | ||
| if (enableTouchRipple && !rippleRef.current) { | ||
| console.error( | ||
| [ | ||
| 'MUI: The `component` prop provided to ButtonBase is invalid.', | ||
| 'Please make sure the children prop is rendered in this custom component.', | ||
| ].join('\n'), | ||
| ); | ||
| } | ||
| }, [enableTouchRipple]); | ||
| } |
There was a problem hiding this comment.
The history: #20401. I guess taking the initial issue reproduction, and adding the warning where the code crash would do it?
github-actions
bot
added
PR: out-of-date
There was a problem hiding this comment.
The changes makes sense, I see there are still some failing tests related to Menu.
Please add a Breaking Change section in the PR description explaining how people can update their tests to be compatible with this change, especially considering how much discussion we had around what would be the best way to test now that the Ripple is lazy loaded :)
github-actions
bot
removed
the
PR: out-of-date
|
The changes are complete and ready for review. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
| @@ -296,6 +298,7 @@ function render( | |||
| ); | |||
| const result: MuiRenderResult = { | |||
| ...testingLibraryRenderResult, | |||
| user: userEvent.setup(), | |||
There was a problem hiding this comment.
This will be super useful for all tests in the future 🎉
There was a problem hiding this comment.
Yes I'll post about it in slack once this is merged.
There was a problem hiding this comment.
This looks wrong in two separate ways:
- The API methods exposed from
render()are supposed to be scoped to the element. It's not the case withuser. - The testing strategy contradicts [code-infra] Add react-testing-library eslint plugin mui-public#173 where the objective is to use a global import to simplify the testing DX.
It feels like we should deprecate this:
| user: userEvent.setup(), | |
| /** @deprecated import { user } instead */ | |
| user: userEvent.setup(), |
and we export user as a module like we do for screen.
cc @Janpot for awareness. I landed here from: mui/mui-x#14142 (comment) from KevinVandy/material-react-table#1231 from https://x.com/XCSme/status/1829522518517956996
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
Nice 👍
Because ripple actions now run asynchronously, tests running in jsdom now require fireEvent calls to be wrapped as such to avoid triggering React warnings:
If too many developers complain about this breaking change, I think that we could disable the ripple when NODE_ENV === 'test'.
Helps #35716
Breaking changes
Because ripple actions now run asynchronously, tests running in jsdom now require
fireEventcalls to be wrapped as such to avoid triggering React warnings:Context
I've been working on the data grid's performance, and I've noticed that the Material UI components we use are causing performance issues during scrolling. The cause of them is the Button's Ripple child, which is mounted in a 2nd render. This triggers re-renders that compete with scroll event handlers, which are already on a tight budget (16ms to respond to scroll events). The graph below shows those re-renders, the 4-5ms stacks that follow the
scrollevents.Solution
From the code comments, I can understand that the ripple is rendered in a 2nd render to avoid the server-side render cost of the ripple component. This PoC solves both the server-side and client-side costs by delaying rendering the ripple until it is actually needed.
The following graphs show the flamecharts for this benchmark.
Note: the click event runtime cost is irrelevant, the disappearance of the 2nd render is the important part.
Open questions
The code in this PR is a draft, if you can give me the greenlight for this change I'll clean it up. I'm not sure where else the ripple is used, but it would probably be good to use this pattern in all those other uses as well, but I'm not sure if you'd like that to be done in this PR. Let me know what scope this PR should include. I also see that you have a
nextbranch and amui-material-nextfolder, I'm not sure how to proceed and what I should edit.