Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: App crash on fast browser navigation #1757

Closed
ghost opened this issue Feb 1, 2022 · 25 comments
Closed

[Bug]: App crash on fast browser navigation #1757

ghost opened this issue Feb 1, 2022 · 25 comments
Labels
bug Something isn't working hydration

Comments

@ghost
Copy link

ghost commented Feb 1, 2022

What version of Remix are you using?

1.1.3

What version of Node are you using? Minimum supported version is 14.

16.0.0

Steps to Reproduce

  1. Create a new Remix app with npx create-remix@latest.
  2. Choose the Remix dev server.
  3. Install dependencies using yarn install.
  4. Next to app/routes/index.tsx, add app/routes/other.tsx with a component that simply renders <>Other</>
  5. In app/routes/index.tsx, add <Link to="/other">Other</Link> somewhere, with import { Link } from "remix";.
  6. Run your app using yarn dev or yarn start; the error happens for both dev and prod builds.
  7. Open your browser and create a new browser tab (the bug happens with both Firefox and Chrome).
  8. Go to your app's URL.
  9. Click the link to Other.
  10. Click the browser back button two times. The tab should now show your browser's default UI for new tabs.
  11. Now very quickly click the browser's forward button twice.
  12. The app should now crash. If it doesn't you didn't click the forward button fast enough.

Expected Behavior

The page content should be displayed without any errors.

Actual Behavior

In the simple repo above, the app crashes and there is the following stack trace in the dev console. In my real app in a production build, this issue then causes #1678, basically making the entire browser tab unresponsive.

components.js:470 Uncaught TypeError: Cannot read properties of undefined (reading 'meta')
    at Meta (components.js:470:21)
    at renderWithHooks (react-dom.development.js:14985:18)
    at mountIndeterminateComponent (react-dom.development.js:17811:13)
    at beginWork (react-dom.development.js:19049:16)
    at HTMLUnknownElement.callCallback2 (react-dom.development.js:3945:14)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:3994:16)
    at invokeGuardedCallback (react-dom.development.js:4056:31)
    at beginWork$1 (react-dom.development.js:23964:7)
    at performUnitOfWork (react-dom.development.js:22776:12)
    at workLoopSync (react-dom.development.js:22707:5)
Meta @ components.js:470
renderWithHooks @ react-dom.development.js:14985
mountIndeterminateComponent @ react-dom.development.js:17811
beginWork @ react-dom.development.js:19049
callCallback2 @ react-dom.development.js:3945
invokeGuardedCallbackDev @ react-dom.development.js:3994
invokeGuardedCallback @ react-dom.development.js:4056
beginWork$1 @ react-dom.development.js:23964
performUnitOfWork @ react-dom.development.js:22776
workLoopSync @ react-dom.development.js:22707
renderRootSync @ react-dom.development.js:22670
performSyncWorkOnRoot @ react-dom.development.js:22293
scheduleUpdateOnFiber @ react-dom.development.js:21881
updateContainer @ react-dom.development.js:25482
(anonymous) @ react-dom.development.js:26021
unbatchedUpdates @ react-dom.development.js:22431
legacyRenderSubtreeIntoContainer @ react-dom.development.js:26020
hydrate2 @ react-dom.development.js:26086
(anonymous) @ entry.client.tsx:4```
@ghost ghost added the bug Something isn't working label Feb 1, 2022
@revelt
Copy link
Contributor

revelt commented Feb 2, 2022

#1618 might be related to this

@axel-habermaier
Copy link
Contributor

axel-habermaier commented Feb 20, 2022

This seems to be caused by a race condition in the Remix code.

As far as I understand, the loadRouteModule function in routeModules.ts loads the JavaScript file related to a route and then adds an entry to the routeModules cache.

The Meta component uses this cache to do its thing.

If you're doing the browser navigation slowly, everything works as expected: loadRouteModule loads the JS file, updates the cache and only then does Meta try to use it. If you're fast, however, loadRouteModule hasn't updated the routeModules cache yet, but Meta tries to access it anyway, resulting in the observed crash.

I'm not sure how to fix this, the code that is involved is quite... complicated and I've not yet been able to completely understand it.

@axel-habermaier
Copy link
Contributor

axel-habermaier commented Feb 20, 2022

After further experimentation, it is quite clear that React Router immediately wants to render app/routes/other.tsx (the second route in the repo case above), completely skipping rendering the first route. But the second route's loader hasn't been executed yet.

This can easily be seen by adding a console.log statements to loadRouteModule and by adding console.log("rendering", useMatches()) to the Routes function in components.tsx

Update

Actually, it's the browser that skips the first page. This can be seen by adding a console.log to the root document in a script tag.

So it seems that the app can be reached via "server-side" navigation where the Remix code expected it to be reachable via client-side navigation only. In other words: The SSR state is invalid for the route that is supposed to be shown by the client-side code and the code that is supposed to update the SSR state doesn't get executed.

So that would mean that before hydration, Remix should check whether it needs to update the SSR state, e.g., downloading additional JavaScript and updating the routeModules cache plus potentially other things that get updated that I'm not aware of.

@ryanflorence, @kentcdodds Any thoughts on what would be a good strategy to tackle this issue?

@tomslutsky
Copy link

I just faced the same issue.
Did someone found a solution for that?

@fnebenfuehr
Copy link

fnebenfuehr commented May 16, 2022

A similar error occurred when I stupidly rendered <Scripts/> twice in root.tsx. Maybe this is helpful for someone.

import {
  Links,
  LiveReload,
  Meta,
  Outlet,
  Scripts,
  ScrollRestoration,
} from "@remix-run/react";

export default function App() {
  return (
    <html>
      <head>
        <Meta />
        <Links />
      </head>
      <body>
        <Outlet />
        <ScrollRestoration />
        <Scripts />
        <LiveReload />
        <Scripts /> {/* Duplicate! */}
      </body>
    </html>
  );
}

components:js:

Uncaught TypeError: Cannot destructure property 'default' of 'routeModules[id]' as it is undefined.
    at RemixRoute (components.js:179:14)
    at renderWithHooks (react-dom.development.js:14985:18)
    at updateFunctionComponent (react-dom.development.js:17356:20)
    at beginWork (react-dom.development.js:19063:16)
    at HTMLUnknownElement.callCallback2 (react-dom.development.js:3945:14)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:3994:16)
    at invokeGuardedCallback (react-dom.development.js:4056:31)
    at beginWork$1 (react-dom.development.js:23964:7)
    at performUnitOfWork (react-dom.development.js:22776:12)
    at workLoopSync (react-dom.development.js:22707:5)

The error occurs only on <Link/> navigation. On refresh the route loads fine.

@designbyadrian
Copy link

I'm also getting a components.js:176 Uncaught TypeError: Cannot destructure property 'default' of 'routeModules[id]' as it is undefined. at RemixRoute (components.js:176:14), but having a hard time telling when. Still investigating.

@Manuelbaun
Copy link

Manuelbaun commented Jul 9, 2022

@f1ow thank you so much, i had the same issue, and also rendered Scripts twice!
The team could maybe add some errors, when used the Script tag more the once?

@seyaobey-dev
Copy link

seyaobey-dev commented Jul 31, 2022

I encountered same issue today with latest @remix-run/react

@synthlace
Copy link

Same. The error appears on the first hydration with a quick navigation to another page (in my case, a nested one).

@nabeelpkl
Copy link

any update on this?

@n8agrin
Copy link
Contributor

n8agrin commented Nov 23, 2022

I'm running into this as well.

A trivial repro is:

  1. clone the remix repository and create the default playground app.
  2. cd to playground/ start it
  3. open localhost:3000, in chrome set your network speed to fast 3g
  4. click the "signup" link
  5. click back twice, so you're on the default chrome page (no url loaded)
  6. click browser forward button once, and before entry.client.tsx loads, click forward again
  7. url should now be localhost:3000/join
  8. 💥 app crashes

hypothesis:

  1. routeModules is loaded from the the root route
  2. when entry.client.tsx finishes loading it tries to load the route module for /join which is not part of the routeModules map and it blows up

hypothetical fix:

  1. before running any client code, ensure the route module for the given route is loaded

@elledienne
Copy link

Running into the same issue caused by fast navigation between pages

@daxaxelrod
Copy link

Any ideas? I don't have a duplicate <Script /> or another other top level tag. Occurred when I was showing off remix to my boss 😅

@n8agrin
Copy link
Contributor

n8agrin commented Jan 20, 2023

Crafted a PR that demonstrates a repro case for this issue: #5166

cc/ @jacob-ebey

@brophdawg11
Copy link
Contributor

I think I'm gonna call this the goldilocks bug. It's not just an issue of clicking too fast, or too slow. It seems to happen if you click forward, then wait just the right amount of time for the modules to start loading for index.tsx and then click the forward button a second time during the index chunk loads.

I could never reproduce it normally, but once I turned on network throttling and waited long enough to see the .js files start loading I could reproduce it.

I believe this is what's happening:

  • Forward click - Remix renders / (index route) on the server
    • Remix sends back the proper modules for entry-client and the index route
    • Network requests go out for entry-client and index
  • While they are loading, second forward click
    • popstate is ignored since we haven't finished loading the chunks yet and therefore haven't instantiated our client side router and it's popstate listener
    • Once the chunks load and go to hydrate, it attempts to hydrate /other since that's the current URL

A few off-the-cuff options:

  1. Easier fix - track the URL at page load and if it's not the same at hydration start (once JS chunks have loaded) we force a hard reload of /other
  2. Harder fix - same detection, but instead of a hard reload we use the manifest to kick off loads for the /other chunks and delay hydration until they complete. That introduces the case for the same issue on a third click, so that would have to do the check each time the new "initial" route chunks load.

I prefer 1 :)

@n8agrin
Copy link
Contributor

n8agrin commented Mar 2, 2023

@brophdawg11 any chance there is an update on this? We're still running into the issue which causes the app to arbitrarily crash:

image

n8agrin added a commit to n8agrin/remix that referenced this issue May 16, 2023
… route

This can happen when entry.client.ts isn't fully loaded before a route change occurs.

See remix-run#1757 for more info.
@n8agrin
Copy link
Contributor

n8agrin commented May 16, 2023

Added a PR with suggested fix here: #6409

@armando-herastang
Copy link

armando-herastang commented May 23, 2023

Having issues with this as well. Commenting to track

@brophdawg11 brophdawg11 added the awaiting release This issue has been fixed and will be released soon label Jun 14, 2023
@brophdawg11
Copy link
Contributor

This is fixed by #6409 and should be available once the next release goes out (should be 1.18.0)

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-ad9adee-20230615 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-12440f3-20230616 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.18.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11
Copy link
Contributor

I did some digging back into this today after we made the change in #9695 and reverted it because it caused some other regressions. I was going to try to dig in and see if there was a better fix than the URL check we are currently doing which has some false positives (#8872, #7529) and was having trouble reproducing it.

In my real app in a production build, this issue then causes #1678,

Reading through #1678 though I had forgotten that this looping was only an issue in React 17, and React 18 did not go into an hydration loop. That was the underlying reason we aimed to fix this - because the infinite loop React got into was problematic, not so much the fact that a hydration error occurred. Now that Remix v2 requires React 18, the looping issue is gone.

So, it seems that at this point, the original looping issue from OP here is no longer a concern in Remix v2. And the fix we have in had some false positive edge cases that cause a loop of it's own. So our fix, applied to React 18, introduces a potential loop which is worse than the actual non-looping hydration error which is fixed by a reload.

So I'm inclined to remove this URL check and, if desired, we can potentially look for a more appropriate fix as part of #7080. But honestly I think the hydration errors that may occur if we did remove this check are likely less of an issue than the current loops we get into from false-positives.

@brophdawg11
Copy link
Contributor

brophdawg11 commented Aug 20, 2024

FYI we just merged #9890 to remove the hydration check originally added in 1.18.0. Details are in the #9890 changeset and the comment above.

@jacob-ebey and I then spent a while reproducing the issue on various versions of Remix (back to 1.17.0) and React (17 and 18) and from what we can tell, current Remix 2.11.2 no longer has the original issue any longer. I am pretty confident it's the usage of React Router's route.lazy as suspected in #7080 (comment) and the presence of HydrateFallback in Remix these days. I also think that as part of the fog of war implementation we had to make all accesses to the routeModulesCache defensive since modules are not guaranteed to be in there and that eliminated some of the client-side rendering errors that triggered the error boundaries in the first place.

When you SSR / and then try to hydrate against something else (/page), Remix realizes it doesn't have the right modules/data to initialize and runs the route.lazy implementation for the missing routes. This causes the client side router to try to hydrated with the default root HydrateFallback (or your own if you happened to have one). This doesn't match what got SSR'd so you get a React hydration error and There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering. which then replaces the SSR'd content with the default HydrateFallback. Then once the proper modules load and route.lazy resolves, the client side switches over to the proper page contents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hydration
Projects
None yet
Development

No branches or pull requests

15 participants