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

App router causes unexpected page reload on popstate event #56636

Closed
1 task done
cduff opened this issue Oct 10, 2023 · 7 comments
Closed
1 task done

App router causes unexpected page reload on popstate event #56636

cduff opened this issue Oct 10, 2023 · 7 comments
Labels
area: Routing Routing (next/router, next/navigation, next/link) bug Issue was opened via the bug report template. locked

Comments

@cduff
Copy link

cduff commented Oct 10, 2023

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/next-app-router-spa-reload-issue-dnzlt9

To Reproduce

  1. npm run dev
  2. Click "link to path" and "link to home" a few times, noting URL path changing and "pathname change count" incrementing.
  3. Click the (embedded) browser back button. Note the navigation completes, the count increments, but then an unwanted full page reload occurs, with the counter resetting.

To see my current workaround

  1. In layout.tsx, uncomment the //if (type === "popstate"... line.
  2. Redo the above steps. Note that the browser back/forward buttons now work as expected with no unwanted page reloads.

Current vs. Expected behavior

Current behavior

I've migrated an SPA from create-react-app to Next.js and referenced these instructions:

https://nextjs.org/docs/app/building-your-application/upgrading/from-vite

It's working fine in general, but I've noticed unwanted/unnecessary page reloads after the browser back button is used. These impact our app performance and analytics.

The SPA currently makes use of react-router-dom for client-side routing. I may look to incrementally migrate this to the Next.js app router in the future.

I think I've identified the culprit code here:

Where the popstate event has state and a falsy __NA value, it assumes the history entry was pushed by the 'pages' router, but this is not necessarily the case.

Expected behavior

  • app-router more correctly identifies history entry pushed by pages router and ignores entries pushed 'outside' Next.js, or;
  • App router enhanced to support beforePopState like it was for the pages router, so that false can be returned to avoid this bug.

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
  Platform: linux
  Arch: x64
  Version: #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023
Binaries:
  Node: 16.17.0
  npm: 8.15.0
  Yarn: 1.22.19
  pnpm: 7.1.0
Relevant Packages:
  next: 13.5.5-canary.4
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.2.2
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

App Router, Routing (next/router, next/navigation, next/link)

Additional context

No response

@cduff
Copy link
Author

cduff commented Nov 17, 2023

Looks like this might be getting fixed by experimental: { windowHistorySupport: true } in next.config.js as well as bug fixes that landed in 14.0.4-canary.1, namely #58438.

@timneutkens
Copy link
Member

Yeah windowHistorySupport will solve your case as well, previously manually pushing/replacing using window.history was not supported 👍 So this wasn't a bug, more of a feature request.

@cduff
Copy link
Author

cduff commented Nov 17, 2023

Yeah windowHistorySupport will solve your case as well, previously manually pushing/replacing using window.history was not supported 👍 So this wasn't a bug, more of a feature request.

Thanks @timneutkens. The documentation at https://nextjs.org/docs/app/building-your-application/upgrading/from-vite#step-5-create-the-entrypoint-page led me to believe it should be possible to migrate a fully client-rendered SPA to Next.js.

@timneutkens
Copy link
Member

It is, but not when you were using react-router history integration, using react-router with memory integration has always worked though. With the new experimental flag both work.

@artola
Copy link

artola commented Nov 22, 2023

It is, but not when you were using react-router history integration, using react-router with memory integration has always worked though. With the new experimental flag both work.

@timneutkens I love this 'feature.' I need to migrate 40-50 web apps using react-router-dom, and this is a game changer. I can navigate backward and forward without losing the state. Of course, this is not our goal, just a step in the right direction, to upgrade such a legacy burden.

@cduff
Copy link
Author

cduff commented Dec 8, 2023

Confirmed fixed in 14.0.4 with experimental: { windowHistorySupport: true }. 👍

@cduff cduff closed this as completed Dec 8, 2023
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: Routing Routing (next/router, next/navigation, next/link) bug Issue was opened via the bug report template. locked
Projects
None yet
Development

No branches or pull requests

3 participants