Performance issue with Modals in Tests (JSDOM)

[ghostling]

We recently build a lot of modals that contain form elements and noticed that the unit/integration tests for them show that they're really slow. Upon investigating in the browser, it seems that the Modal components get rerendered when form elements inside ModalBody changes.

One slight refactor (shown below) makes the same tests go 10x faster:

// Testing this component is slow (20s in test):
<Modal>
  <ModalHeader />
  <ModalBody>
     <Form /> // Approx ~10 form inputs, 1 combobox
  </ModalBody>
  <ModalFooter />
</Modal>

// Just doing this speeds it up by 10x (2s test):
// Parent:
<Modal>
  <ModalHeader />
  <CustomModalBody />
</Modal>

// CustomModalBody:
<>
  <ModalBody />
  <ModalFooter />
</>

I also have a small codesandbox example where you can see the rerenders using React Dev Tools: https://codesandbox.io/s/distracted-waterfall-o5moc?file=/src/index.tsx .

Additional context is that we want to do validation on all the form elements + submit button. I recognize that this is an old pattern; we just haven't had the time to redesign it. That shouldn't distract us from Modal performance as we see slow tests in modals that don't contain form elements either.

We use React Testing Library for our integration tests and user-event to interact with forms in the test.

[TheSisb]

Thanks for the detailed report @ghostling! Added a ticket to investigate further into our backlog.

[TheSisb]

Hi again @ghostling,

I had a chance to look into your slowness issue and believe I figured it out. I've created a fork of your Codesandbox with a recommended solution. Here's a breakdown of my takeaways:

1- The majority of the slowness in SlowModal is because it is handling the form state in the same component that renders the Modal wrapper (see line 20 and line 40 of SlowModal). Whenever someone types in a field, the form state updates and it re-renders the entire Modal wrapper and everything within it. Since the form state is only concerned with what's inside the Modal wrapper, we can (and arguably should) move that state inside the Modal component. I do this in the RecommendedModal solution in my fork, and it's also done in FasterModal as a side-effect of your experiments. It's always wise to keep state as deep into the React tree as possible due to how React's component paradigm updates nodes.

2- The <Modal> component is kind of special; it handles showing and hiding the dark backdrop and the entire Modal contents via a React Portal on the page. For this reason, we don't need to conditionally render it like other components. As you can see in RecommendedModal and on our docs examples, it will handle its own conditional rendering. This may provide another slight perf improvement as well.

3- A helpful nit: within the RecommendedModalForm component and unlike FasterModal, I've replaced the wrapping React Fragment (<>) with a form element. Providing a form element with an onSubmit will make your form submittable by hitting the enter key. I changed the SubmitButton to be of type="submit" (it defaults to "button") so that it can inform the form that its action activates the form onSubmit. It's disabled state also prevents submitting the form natively.

I'm going to make a ticket to update our doc site to provide more insight into this best practice.

Hope this helps!

[SiTaggart]

So I think I've narrowed it down to focusable elements in a modal. Playing with your sandbox, if you just add a bunch of plain html buttons, it starts blowing out the test times. Regardless of state or anything else. Just focusable elements, doesn't even have to be Paste components.

Interestingly the performance isn't degraded once it's in the browser though. So it seems scoped to the test environment.

That's the update so far. We'll look into it more and see if there's an easy solution

[ghostling]

Hey @SiTaggart or @TheSisb , are there any updates for this?

[SiTaggart]

No updates as of right now. The ticket is in our backlog and we'll try to prioritize it in up coming sprints.

[TheSisb]

+1 to what Simon said. End of 2020 was busy, but we'll be looking into this again very soon!

[ghostling]

Totally understand, appreciate the update!

[richbachman]

Hi @ghostling

We finally had some time to investigate this further. Here's a write up the findings:

Initially, I setup multiple CodeSandboxes to see if the test performance issue was in our implementation or the Reach/Dialog implementation. In those, I setup three modal components:

• Modal with one line of content
• Modal with multiple lines of content
• Modal with about 10 buttons and an input

I then ran a simple render test to see how long the tests took to run. In all samples, once a modal had focusable content, the test times increased to above 1000ms. This happened in the Paste implementations and the base Reach/Dialog implementation. So I came to the conclusion that the issue was with Reach/Dialog.

After that conclusion, I started down the path of swapping out Reach/Dialog for Reakit Dialog in our primitive. I chose Reakit because we were already using other Reakit components for primitives, and those have been successful. I created a PR for that and then started a PR to change the Modal to use the new primitive. I soon realized that changing to Reakit would introduce too many break changes to Modal. After discussing with the team, we came to the conclusion that we shouldn't switch to Reakit and maintain the current Modal API, and not break any downstream team trust.

I then did some more digging into Reach/Dialog and found their tests ran fairly quick even with focusable content in the dialog. Then I realized we were a few versions behind on their dialog, and they were also testing their dialog with React 17. They have a method of testing their components with React 16, and after running that I found that the tests run considerably slower in React 16: https://share.getcloudapp.com/rRuGr5om (top example is React 16)

After that I set down the path of testing to see if upgrading Reach/Dialog and then upgrading to React 17 would solve the performance. I tested the current Paste version, upgrading Reach/Dialog, and then upgrading Reach/Dialog and React. In each test I ran the test suite 2x. One with a modal without focusable content (top), and one with about 10 buttons and an input (bottom). Screenshots are below:

• Baseline using current Paste version: https://share.getcloudapp.com/WnuYOpOQ/
• Upgraded Reach/Dialog: https://share.getcloudapp.com/v1ubXnR5/
• Upgraded Reach/Dialog and React 17: https://share.getcloudapp.com/rRuG2Bow/

As you can see once we upgrade Reach/Dialog we get a big improvement in our test performance. 1195ms down to 344ms for the modal with focusable content. We get a very slight improvement when we upgrade to React 17 as well.

For now upgrading the React/Dialog version should speed up our tests and any downstream team tests. We'll upgrade to React 17 at a later date.

[SiTaggart]

Any update to the modal has just gone out @ghostling, let us know if you see an improvement

[ghostling]

Hey @richbachman , thanks for the stellar writeup 💯 and also for fixing this issue. I'll have to coordinate with my team to do some Paste upgrades first 😅 but looking forward to these faster tests!

[ghostling]

Wow, took me a while to circle back 😅 , so the Paste upgrades did not improve the test speed very much, but we found that mocking out Modals in our tests really helps, hoping to get some feedback on this approach:

// NOTE: we're using unbarreled imports, so using jest module mock pattern here:
// __mocks__/@twilio-paste/core/modal/index.tsx:
import React from 'react';

interface Props {
  'data-s': string;
  role: string;
  children: React.ReactElement;
}

const Div = (props: Props & any) => (
  <div role={props.role} data-s={...props['data-s']}>
    {props.children}
  </div>
);

const Modal = (props: any) => <Div role="dialog" {...props} />;
const ModalBody = Div;
const ModalFooter = Div;
const ModalFooterActions = Div;
const ModalHeader = Div;
const ModalHeading = (props: any) => <Div role="heading" {...props} />;

export { Modal, ModalBody, ModalFooter, ModalFooterActions, ModalHeader, ModalHeading };

In the mock, we just made everything a div and kept role/data-s tags to make it easy to select on for presence. Propagating the props don't really do much as I found that the onDismiss goes from the <Modal /> to the <ModalHeading /> via a Context, so we weren't able to keep our tests that test closing the modal via the "x", but it's not a deal breaker for us.

Example timing:

// Before mocks
PASS src/ts/components/naps/__tests__/CreateNapModal.test.tsx (30.07 s)
  <CreateNapModal />
    ✓ Cancels creation of NAP (1254 ms)
    ✓ Can change unique name and assert correct value (5819 ms)
    ✓ Fails Nap creation with an invalid name (21440 ms)

// After mocks:
PASS src/ts/components/naps/__tests__/CreateNapModal.test.tsx
  <CreateNapModal />
    ✓ Cancels creation of NAP (334 ms)
    ✓ Can change unique name and assert correct value (820 ms)
    ✓ Fails Nap creation with an invalid name (850 ms)

[kate-porter]

Hey @ghostling! I've just shared this with @twilio-labs/design-systems-eng and we'll get back to you soon.

[TheSisb]

Hi @ghostling,

If this worked for you this would be fine. You can trust that Paste is testing the Modal, so this is a good way to avoid re-testing the modal and just testing your business logic.