Fix: Router white flash on first navigation to page #3772

Tobbe · 2021-11-22T14:46:34Z

This is an alternative attempt at a solution for #2124

When I was reviewing @callingmedic911's PR for the same issue (#3767) I did some refactoring while getting myself up-to-speed on the code again. And while doing so (and after reading @jtoar's excellent link to the React beta docs) I came up with a (in my eyes) simpler solution to the problem.

This PR has two commits (as of writing this). The first one is just the refactoring I did. The second one is the actual solution to the linked issue.

The TL;DR of it is: Set a hard-coded key on the <Route>s so that <PageLoader> can keep its state while navigating.

Need to update the docs: https://redwoodjs.com/docs/prerender#flash-after-page-load

callingmedic911 · 2021-11-22T15:17:28Z

@Tobbe I checked-out this locally, this is a neat solution! But I'm afraid unmounting could still happen if navigation is not between the Route to Route. It could be from the old Route to a Set which contains the new route. Here's a small repoduction:

<Router>
  <Set wrap={TestWrapper}>
    <Route path="/second" page={SecondPage} name="second" />
  </Set>
  <Route path="/first" page={FirstPage} name="first" />
  <Route notfound page={NotFoundPage} />
</Router>

From what I understood, if it was only possible to wrap Set once, even we could have handled it with simple control flow but Set could be multi-level nested. Can we somehow retain the route state even if parents change?

Tobbe · 2021-11-22T19:20:51Z

It could be from the old Route to a Set which contains the new route

Yes, I considered this too.

Here's what I'm afraid will happen if we have a "global" Page. But I haven't fully verified.

The PageLoader is only for the actual page, not the Set and the wrappers around it.
So if you jump from a page in BlogLayout to a page in AdminLayout you'd get the old page rendered inside AdminLayout until pageLoadingDelay ms has passed, or the new page has finished downloading. I thought this would be weird, so I made sure everything was unmounted as soon as you "leave" the current Set

callingmedic911 · 2021-11-22T19:33:52Z

The PageLoader is only for the actual page, not the Set and the wrappers around it. So if you jump from a page in BlogLayout to a page in AdminLayout you'd get the old page rendered inside AdminLayout until pageLoadingDelay ms has passed, or the new page has finished downloading. I thought this would be weird, so I made sure everything was unmounted as soon as you "leave" the current Set

You're right, that would be a very inconsistent state. Worse than the white screen IMO.

Yes, I considered this too.

I see the white page for this case, am I missing something here?

Tobbe · 2021-11-22T19:40:47Z

I see the white page for this case, am I missing something here?

Sorry, English is difficult... I just meant I knew about the scenario you mentioned. And that I thought the white flash was better than the alternative I described.

callingmedic911 · 2021-11-22T19:46:03Z

Oh, I see. Would love to know your thought on this: #3767 (comment)
Specifically:

Long term fix can be a bit of refactoring. Theoretically, we have to put Loader outside of Route/activeChildren and pass download page module as a prop to the current Route. Then instead of returning PageLoader from InternalRoute, it can return Page directly. But this seemed too much code change considering we are very near to v1. However, if we agree to it I can proceed with guidance from you guys - @Tobbe @jtoar.

Having PageLoader outside will hold the "swapping" process (including Sets) until the page is ready. What do you think? Too much change?

Tobbe · 2021-11-23T00:59:07Z

Having PageLoader outside will hold the "swapping" process (including Sets) until the page is ready. What do you think? Too much change?

I felt I couldn't get a good grip on how much work this would be, so I just got started on it. And it doesn't look like it'll be too bad. I'll spend a few hours more on it tomorrow to finish it off.

Tobbe · 2021-11-26T15:09:58Z

it doesn't look like it'll be too bad. I'll spend a few hours more on it tomorrow to finish it off.

Hah! That wasn't true. This turned out to be a much bigger and complicated change than I first thought.

@jtoar This touches on the a11y stuff you've built. Had to update the Set test snapshots. Can you please take a look?
@dac09 pre-rendering died due to my changes. I've gotten them back to life, but please do take a look. How much confidence do you have in our pre-render tests?

dac09 · 2021-11-26T15:30:53Z

Prerender tests are pretty solid!

Tobbe · 2021-11-26T15:49:30Z

Awesome! That's a relief 🙂

Tobbe · 2021-11-26T18:56:50Z

@callingmedic911 I'd love your review of this 🙏

callingmedic911 · 2021-11-26T19:02:49Z

@callingmedic911 I'd love your review of this 🙏

Glad to. Huge props to you for pulling this big of a change this fast. 🙌 🚀 I'll try to squeeze some time this weekend to review.

jtoar

Awesome job @Tobbe! This one's a doozy. I'll have to take more than one pass at reviewing it.

First off, super happy that ActiveRouteLoader is a function component! When I was digging into React 18 I was worried that it might take a lot of nontrivial work to get the Router concurrent-mode/suspense ready. There's still some work to do but I'm feeling a lot better about it now that it's a function component.

I've tried this PR out locally on an app I have. The behavior's definitely fixed, and I haven't caught any regressions yet.

After this PR, I think we need to take some time to organize some of the code in the router package. For example, path-validation logic seems to be duplicated in a few places; it'd be easier to follow if it was centralized. Moreover, we need a legit "understanding the router" or contributing guide.

packages/router/src/active-route-loader.tsx

jtoar · 2021-11-29T00:45:34Z

packages/router/src/active-route-loader.tsx

+  if (global.__REDWOOD__PRERENDERING) {
+    // babel auto-loader plugin uses withStaticImport in prerender mode
+    // override the types for this condition
+    const syncPageLoader = spec.loader as unknown as synchronousLoaderSpec
+    const PageFromLoader = syncPageLoader().default
+
+    const prerenderLoadingState: LoadingStateRecord = {
+      [path]: {
+        state: 'DONE',
+        page: PageFromLoader,
+      },
+    }


It's tempting to want to move this up and "return early", but (I think) it actually needs to be here because we can't call hooks conditionally. That's all just to say that it may be worth adding a comment here explaining why it's last.

Done. Please lmk what you think

@jtoar Have you had a chance to look at this yet?

packages/router/src/active-route-loader.tsx

Tobbe · 2021-11-30T05:24:21Z

~~The CI jobs keeps failing to install. Not sure how to get this unstuck~~
It was a broken yarn.lock file. Now fixed

jtoar

This looks good to go! I feel like this really opens the door to even more refactoring, like you brought up in your page loader issue (I think we could consolidate “validatePaths” too—I’ll make a few follow-up issues myself).

I’m really impressed by all the work you did with the router tests. I thought that we might have to be perennially on-the-look-out for the flash, but you’ve seriously tested it every which way. I’ll try to take a page out of your book when writing tests going forward.

I’ll leave you to merge 🚀

redwoodjs-bot bot assigned thedavidprice Nov 22, 2021

redwoodjs-bot bot added the triage/processing label Nov 22, 2021

Tobbe added the topic/router-&-navigation label Nov 22, 2021

thedavidprice removed their assignment Nov 22, 2021

thedavidprice removed the triage/processing label Nov 22, 2021

thedavidprice marked this pull request as draft November 22, 2021 17:52

Tobbe force-pushed the tobbe-router-white-flash branch 2 times, most recently from 737863d to e3e5951 Compare November 22, 2021 23:26

Tobbe force-pushed the tobbe-router-white-flash branch 2 times, most recently from 7831b05 to 49f0077 Compare November 26, 2021 14:53

Tobbe marked this pull request as ready for review November 26, 2021 18:52

callingmedic911 mentioned this pull request Nov 26, 2021

Fix: Flickering page on route navigation #3767

Closed

3 tasks

jtoar reviewed Nov 29, 2021

View reviewed changes

Tobbe force-pushed the tobbe-router-white-flash branch 3 times, most recently from 15b1a3a to e20e26b Compare November 30, 2021 05:00

Tobbe added the next-release/update-docs label Dec 1, 2021

thedavidprice assigned jtoar Dec 2, 2021

jtoar mentioned this pull request Dec 20, 2021

Still allow generic Errors in redwood forms #3946

Merged

Tobbe mentioned this pull request Dec 20, 2021

Get rid of PageLoader #3957

Closed

jtoar approved these changes Dec 21, 2021

View reviewed changes

Tobbe added 18 commits December 21, 2021 05:58

Router: Make analyzeRouteTree self-contained

31352f4

Router: Prevent white flash when downloading pages

8c5dc1a

Router: Instead of returning single-element-array, return element

75efee9

active-route-loader

812d1c8

Update outdated comment

ee412b4

Undo unneeded change

064540e

Don't directly modify state

4e8c07f

useIsMounted

16c1b4f

Comment about early return

3893deb

useIsMounted returns a function

4654b21

Extract contexts

73ac420

router.tsx: Fix import

c853a66

router.tsx: Small tweak of some comments

758a55a

router loading useLocation test

236201d

useIsMounted: Make it easier to read

180a757

router: Unique test names

69a9da4

router: proper path params during loading

294ba97

No need to shallow-clone location

3d6610b

Tobbe force-pushed the tobbe-router-white-flash branch from 0ffc1df to 3d6610b Compare December 21, 2021 04:58

Tobbe enabled auto-merge (squash) December 21, 2021 04:59

Tobbe merged commit 849e047 into redwoodjs:main Dec 21, 2021

redwoodjs-bot bot added this to the next-release milestone Dec 21, 2021

Tobbe deleted the tobbe-router-white-flash branch December 21, 2021 05:13

thedavidprice modified the milestones: next-release, v0.41.0 Dec 21, 2021

jtoar mentioned this pull request Dec 22, 2021

[Router] improve comments and consolidate validation logic #3977

Closed

o0charlie0o mentioned this pull request Dec 27, 2021

Navigating to a route not previously visited flashes white #2124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Router white flash on first navigation to page #3772

Fix: Router white flash on first navigation to page #3772

Tobbe commented Nov 22, 2021 •

edited

Loading

callingmedic911 commented Nov 22, 2021

Tobbe commented Nov 22, 2021

callingmedic911 commented Nov 22, 2021

Tobbe commented Nov 22, 2021

callingmedic911 commented Nov 22, 2021

Tobbe commented Nov 23, 2021

Tobbe commented Nov 26, 2021

dac09 commented Nov 26, 2021

Tobbe commented Nov 26, 2021

Tobbe commented Nov 26, 2021

callingmedic911 commented Nov 26, 2021

jtoar left a comment

jtoar Nov 29, 2021

Tobbe Nov 29, 2021

Tobbe Dec 18, 2021

Tobbe commented Nov 30, 2021 •

edited

Loading

jtoar left a comment

Fix: Router white flash on first navigation to page #3772

Fix: Router white flash on first navigation to page #3772

Conversation

Tobbe commented Nov 22, 2021 • edited Loading

callingmedic911 commented Nov 22, 2021

Tobbe commented Nov 22, 2021

callingmedic911 commented Nov 22, 2021

Tobbe commented Nov 22, 2021

callingmedic911 commented Nov 22, 2021

Tobbe commented Nov 23, 2021

Tobbe commented Nov 26, 2021

dac09 commented Nov 26, 2021

Tobbe commented Nov 26, 2021

Tobbe commented Nov 26, 2021

callingmedic911 commented Nov 26, 2021

jtoar left a comment

Choose a reason for hiding this comment

jtoar Nov 29, 2021

Choose a reason for hiding this comment

Tobbe Nov 29, 2021

Choose a reason for hiding this comment

Tobbe Dec 18, 2021

Choose a reason for hiding this comment

Tobbe commented Nov 30, 2021 • edited Loading

jtoar left a comment

Choose a reason for hiding this comment

Tobbe commented Nov 22, 2021 •

edited

Loading

Tobbe commented Nov 30, 2021 •

edited

Loading