mozilla / perfcompare

Improved Performance Comparison Tool
Mozilla Public License 2.0
39 stars 93 forks source link

Avoid a spurious rerender when changing the framework (and other things related to the router) #700

Closed julienw closed 2 months ago

julienw commented 2 months ago

This issue is especially visible when changing the framework... but only after removing the reloadDocument property, which I'm not doing in this deploy preview, so you won't be able to check this from the deploy preview. You can checkout the pull request locally and remove reloadDocument to check for yourself.

The issue is that the table is rerendered again even though nothing changed yet. From the react profiler, I found that it's rerendered because "context changed", that is the context from react router. This happens when using useLoaderData (see the issue I filed in https://github.com/remix-run/react-router/issues/11864).

So I fixed this through 2 commits:

  1. Commit 1 is the simplest: it uses the "children function" style of Await so that ResultsTable can be a memoized component. Indeed ResultsMain gets the spurious rerender because of its use of useLoaderData.
  2. Commit 2 is a bit more complex. It's goal is to remove the use of useLoaderData in RevisionRow, and more work was needed here. Maybe this could be reverted if https://github.com/remix-run/react-router/issues/11864 is fixed later...
netlify[bot] commented 2 months ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit fb783d1df807b018d48c01934ca0bb34cb095185
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/66ace84a45513100093e2505
Deploy Preview https://deploy-preview-700--mozilla-perfcompare.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

julienw commented 2 months ago

Ah the tests are failing, weird because I tested locally... let me fix that. Update: Should be fixed now

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.84%. Comparing base (2248e6d) to head (fb783d1).

Files Patch % Lines
src/components/CompareResults/RevisionRow.tsx 88.88% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## beta #700 +/- ## ========================================== - Coverage 82.90% 82.84% -0.06% ========================================== Files 82 82 Lines 2094 2093 -1 Branches 379 380 +1 ========================================== - Hits 1736 1734 -2 - Misses 317 318 +1 Partials 41 41 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Carla-Moz commented 2 months ago

@julienw

"From the react profiler, I found that it's rerendered because "context changed", that is the context from react router."

Do you mind showing how you found that in the react profiler? I want to get better at reading profiles in general :P

julienw commented 2 months ago

@julienw

"From the react profiler, I found that it's rerendered because "context changed", that is the context from react router."

Do you mind showing how you found that in the react profiler? I want to get better at reading profiles in general :P

When you look at a recording in the react profiler, you see all the individual renders in the top right rectangle. You can click on one of them. On the view below, all the colored item are the ones that rendered during that render cycle. By hovering them, the profiler tells you why they were rendered: props changed, state changed, context changed, parent rendered, etc. Sometimes you can get some more information (for props I think we know which props have changed), but for contexts not much. So then this requires some digging in the component itself, to guess what might use a context.

That's about it!

julienw commented 2 months ago

Thanks beatrice for the review!