vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.9k stars 26.87k forks source link

Error thrown when window.history.pushState is called with "" as the first argument #68015

Open adamsullovey opened 2 months ago

adamsullovey commented 2 months ago

Link to the code that reproduces this issue

https://codesandbox.io/p/sandbox/github/vercel/next.js/tree/canary/examples/reproduction-template (or https://codesandbox.io/p/devbox/confident-pateu-go8s7s)

To Reproduce

  1. open up the sample Next.js app @ https://codesandbox.io/p/sandbox/github/vercel/next.js/tree/canary/examples/reproduction-template (or https://codesandbox.io/p/devbox/confident-pateu-go8s7s)
  2. open the browser dev tools for the site preview
  3. run window.history.pushState("", null, "/help") in the browser console

Screenshot 2024-07-21 at 5 52 32 PM

Current vs. Expected behavior

Current behaviour

An error is thrown:

Uncaught TypeError: Cannot create property '__NA' on string ''
    at copyNextJsInternalHistoryState (app-router.js:125:19)
    at History.pushState (app-router.js:375:20)
    at <anonymous>:1:16

Expected behaviour

The URL in the navigation bar should change to include the path /help

Provide environment information

This is taken from a local project:

Operating System:
  Platform: darwin
  Arch: x64
  Version: Darwin Kernel Version 23.5.0: Wed May  1 20:09:52 PDT 2024; root:xnu-10063.121.3~5/RELEASE_X86_64
  Available memory (MB): 16384
  Available CPU cores: 16
Binaries:
  Node: 20.12.2
  npm: 10.5.0
  Yarn: 1.22.19
  pnpm: 8.14.1
Relevant Packages:
  next: 14.2.3 // There is a newer version (14.2.5) available, upgrade recommended! 
  eslint-config-next: 14.2.2
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.5.3
Next.js Config:
  output: N/A
 ⚠ There is a newer version (14.2.5) available, upgrade recommended! 
   Please try the latest canary version (`npm install next@canary`) to confirm the issue still exists before creating a new issue.
   Read more - https://nextjs.org/docs/messages/opening-an-issue

The Code Sandbox link above has the same issue and uses Next.js `15.0.0-canary.68`

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

Navigation

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

next dev (local), next start (local), Other (Deployed)

Additional context

I have received a not open-source 3rd party library to integrate with a Next.js site which updates URLs with calls like window.history.pushState("", null, "/some-path"). Passing "" as the first argument should be ok according MDN since "" is serializable. It does not trigger an error outside of Next.js. Unfortunately calling Next.js' version of pushState with a blank string as a first argument causes a crash and the 3rd party library stops working.

The crash originates here: https://github.com/vercel/next.js/blob/e0ed599f43d3a0114e19f7ba6522270e5384e190/packages/next/src/client/components/app-router.tsx#L170

  if (data == null) data = {}
  const currentState = window.history.state
  const __NA = currentState?.__NA
  if (__NA) {
    data.__NA = __NA
  }

If data is "", data will not be reassigned to {}. Later data.__NA = __NA will try to create a new property on "" which is not allowed in strict mode.

I'm happy to put up a PR that changes this if I can get some direction on what the right fix is. I don't know if changing to if (!data) data = {} or if (data == null || data == "") data = {} or something else would preserve more intended behaviours.

icyJoseph commented 2 months ago

You didn't provide the right link to that codesandbox, it is linking to the template starter.

I think the issue here is that Next.js stores its own state into the history, since The state object can be anything that can be serialized.:

{
    "__NA": true,
    "__PRIVATE_NEXTJS_INTERNALS_TREE": [
        "",
        {
            "children": [
                "__PAGE__",
                {}
            ]
        },
        null,
        null,
        true
    ]
}

That's also valid from their side... 🤔

While:

  if (data == null || typeof data === 'string') data = {}

And the link kind of checks, would most likely stop the issue, I wonder what'll happen to the rest of the app. We need a core maintainer up in here.