handling results loading with defer

Carla-Moz commented 3 months ago

This PR handles the loading of the results using React Router's defer method. See here

Closes: Add a spinner when fetching the compare results Add a loading spinner to Compare Button

netlify[bot] commented 3 months ago

Deploy Preview for mozilla-perfcompare ready!

Name	Link
Latest commit	0968a5d189b92175fed24fc28bf9e1e5c9805a61
Latest deploy log	https://app.netlify.com/sites/mozilla-perfcompare/deploys/6695a14423ec010008c5dd2b
Deploy Preview	https://deploy-preview-685--mozilla-perfcompare.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.05%. Comparing base (0eec9c8) to head (448e061).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## beta #685 +/- ## ========================================== + Coverage 91.88% 92.05% +0.16% ========================================== Files 71 71 Lines 1738 1750 +12 Branches 327 329 +2 ========================================== + Hits 1597 1611 +14 + Misses 112 111 -1 + Partials 29 28 -1 ```

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

julienw commented 3 months ago

Thanks, it's much simpler like that! I'm not a big fan of using reloadDocument, but after trying an alternative but still simple way during 1h I couldn't find anything good and working as we'd like to. I left a note in remix-run/react-router#10816 (comment) trying to advocate for an option for this, maybe we'll have something eventually... In the mean time let's move forward and we can always spend some time revisiting this later.

They replied to me and I think it's worth trying their suggestion. That is:

remove the "defer()" call in our loader -- but keep the promise in the result object!
still keep using Suspense/Await
of course remove reloadDocument to test this

If this works, fine, land it. If this doesn't work, fine, land your current solution!

Carla-Moz commented 3 months ago

Thanks, it's much simpler like that! I'm not a big fan of using reloadDocument, but after trying an alternative but still simple way during 1h I couldn't find anything good and working as we'd like to. I left a note in remix-run/react-router#10816 (comment) trying to advocate for an option for this, maybe we'll have something eventually... In the mean time let's move forward and we can always spend some time revisiting this later.

They replied to me and I think it's worth trying their suggestion. That is:
* remove the "defer()" call in our loader -- but keep the promise in the result object!

* still keep using Suspense/Await

* of course remove `reloadDocument` to test this
If this works, fine, land it. If this doesn't work, fine, land your current solution!

Unfortunately their solution doesn't work. It behaves the same way with or without the defer with Suspense/Await and no reloadDocument - meaning the spinner and button loading states show only on initial load not same page user edits. I'll just keep my solution for now and we can revisit for a better one.

mozilla / perfcompare