Fix blank screen flash on client-side redirect

[david-crespo]

Closes #1123

redirect is new in RR 6.4 (it comes from Remix) and it is now the recommended way to do these kinds of redirects, as opposed to rendering <Navigate to="my-path" />. There are some things I don't understand about redirect's behavior that I want to note; otherwise this wouldn't be worth a whole PR.

The phenomena

• Unlike element={<Navigate to="my-path" />}, returning a redirect from a loader does not cause it to hang out on the route while the next loader runs. That's why it fixes Blank screen flash on client-side redirects #1123, which is the point of this PR.
• The navigation appears to be client-side by default, which is what we want
  • This is a little surprising, and I initially resisted using this because I assumed it would be a full pageload, but it is not. When I click the link I get a client-side navigation. The only request that first is the loader for the page we're landing on.
  • This is pretty weird though because if you try to do a full-page nav to, e.g., https://google.com, it will briefly flash a client-side error because that doesn't match any routes you have defined, and then it will actually navigate to google.com. I think this is a bug in RR. It's hard to imagine why you would ever want that to happen.
• The navigation is a replace
  • This is very surprising, and is the other reason I wasn't already using redirect. I did not see a way to make it do a replace instead of a push, which is what we need. It appears to do a replace by default.

Why might this be

First of all, redirect just returns a 302 Response with the passed-in location in the Location header. (source)

The next thing to know is how RR handles a redirect returned from a loader. In callLoaderOrAction, a 302 response is converted into this "result" (source):

return {
  type: ResultType.redirect,
  status,
  location,
  revalidate: result.headers.get("X-Remix-Revalidate") !== null,
};

Finally, in handleLoaders, the comment says redirects are handled as a replace (source) but this is only true if the replace: true is passed to handleLoaders, and as far as I can tell (I went through in the debugger) it is not. Then, of course, in practice it is a replace, but I can't figure out why.

// If any loaders returned a redirect Response, start a new REPLACE navigation
let redirect = findRedirect(results);
if (redirect) {
  let redirectNavigation = getLoaderRedirect(state, redirect);
  await startRedirectNavigation(redirect, redirectNavigation, replace);
  return { shortCircuited: true };
}

So this works for us, but I don't understand why. I will try to find out from the devs. I have a feeling this is only accidentally right, and I want it to be right on purpose.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
console-ui-storybook	✅ Ready (Inspect)	Visit Preview	Aug 23, 2022 at 9:22PM (UTC)

[david-crespo]

This should be the same as the other projectList requests for accurate deduping.

[zephraph]

Feels like we need a less flaky way to ensure these stay in sync.

[david-crespo]

I wholeheartedly agree, I hate it. I have a sneaky idea: if we take the limit param off of the request in QueryTable (unless it's specified through a prop, maybe) then we can rely on the default page size practically everywhere and we avoid syncing it by not using the limit param almost anywhere.

[david-crespo]

On second read, I think I understand what is going on. The key is the { shortCircuited: true }, which happens on the initial navigation to /orgs/:orgName. The old way looked like this:

0. Start on /orgs
1. Click maze-war, triggering history.push('/orgs/maze-war')
2. That route renders <Navigate to="projects" />
3. That triggers a history.replace('/orgs/maze-war/projects')

So: a real nav (push) to /orgs/maze-war followed by a replace taking us to projects. The new model is pretty different:

0. Start on /orgs
1. Click maze-war, start a navigation to /orgs/maze-war
   • Note distinction between "starting" and "completing" a nav. Complete is where history.push or history.replace is called (source)
2. Run the loader that returns redirect('/orgs/maze-war/projects')
   • At this point we have done neither a history.push not a history.replace
3. The fact that the loader returns a redirect causes it to
   1. start a new navigation to /orgs/maze-war/projects
   2. short-circuit, i.e., cancel the old navigation before it can be completed
4. /orgs/maze-war/projects is a normal nav that can be completed as a push

Rather than a push(path 1) -> replace(path 2), from history's point of view there is only push(path 2). The first path never even touches the history. Maybe this is a natural default behavior for precisely the reason we want it: if your loader can return a redirect, and the redirect did a real navigation, hitting back after the redirect would only re-run that loader, redirecting you again. That would be terrible. In any case I hope the final 6.4 docs cover this better. They're actively writing them right now.

[david-crespo]

Aaaaaand of course I'm not done. While the above situation works as desired (not making a history entry for the route we're skipping over), it doesn't work when you hit the route directly. So, for example if you go to /orgs/maze-war (instead of clicking maze-war while on /orgs) you get a history entry for /orgs/maze-war and then another one for the redirect nav /orgs/maze-war/projects. That means if you hit the back button you get the same redirect again, forever. I will figure out what to do.

[david-crespo]

Reverted in 1fda0f3 until I figure this out.