vercel / swr

React Hooks for Data Fetching
https://swr.vercel.app
MIT License
30.54k stars 1.22k forks source link

unstable_serialize doesn't work with NextJS edge runtime #3030

Open william1616 opened 2 weeks ago

william1616 commented 2 weeks ago

Bug report

Description / Observed Behavior

When calling unstable_serialize from a server component that uses the NextJS edge runtime with an array or object key, the key is not serialized properly.

As an example, the code below logs 1~ to the console:

export const runtime = 'edge'

export default function Home() {
  const key = ['test']
  console.log(unstable_serialize(key))

  return "Hello"
}

Expected Behavior

I would expect the behavior in edge runtime to match the NodeJS runtime, with @"test", logged to the console (example code below)

export default function Home() {
  const key = ['test']
  console.log(unstable_serialize(key))

  return "Hello"
}

Additional Context

Debugging

I've done some digging, and the problem seems to originate from the stableHash function in the file: https://github.com/vercel/swr/blob/v2.2.5/src/_internal/utils/hash.ts.

Further testing reveals that although key.constructor doesn't equal Array in the edge runtime, key instanceof Array returns true (same applies to objects).

A possible solution could be to change:

This works in both the edge runtime and NodeJS runtime, but I'm unsure of the historical reasons for not using instanceof. Additionally, I'm not sure why arg.constructor doesn't equal Array or OBJECT in the edge runtime.

Package Versions

nextjs-swr-tests@0.1.0 ├── @types/node@20.16.15 ├── @types/react-dom@18.3.1 ├── @types/react@18.3.12 ├── eslint-config-next@14.2.16 ├── eslint@8.57.1 ├── next@14.2.16 ├── react-dom@18.3.1 ├── react@18.3.1 ├── swr@2.2.5 └── typescript@5.6.3

Repro Steps / Code Example

I've built a test repository that demonstrates the issue. To reproduce:

  1. Clone the repo from https://github.com/william1616/nextjs-swr-edge-runtime-tests.
  2. Install the packages (npm i), and then run the development server (npm run dev).
  3. Open a browser and navigate to http://localhost:3000.
  4. Check the console output:
    • 1~ (the output of unstable_serialize).
    • true (key instanceof Array is true).
    • false (key.constructor == Array is false).
  5. In https://github.com/william1616/nextjs-swr-edge-runtime-tests/blob/main/src/app/page.tsx, comment out line 5 to change the runtime from edge to NodeJS.
  6. Refresh http://localhost:3000 in your browser.
  7. Check the console output:
    • @"test", (the output of unstable_serialize).
    • true (key instanceof Array is true).
    • true (key.constructor == Array is true).
william1616 commented 2 weeks ago

Follow-up

Having done a bit more testing, the reason for using .constructor rather than instanceof is because types like new Set are subclasses of object, and using instanceof would cause subclasses of object to be serialized as objects rather than being added to the weak map.

So my new suggested solution is to use Object.getPrototypeOf — which seems to work in the edge runtime. I'm still unclear why .constructor doesn't work in the edge runtime, though.

Here are some examples:

console.log({}.constructor == Object) // false
console.log([].constructor == Array) // false

console.log({} instanceof Object) // true
console.log([] instanceof Array) // true
console.log(new Set([1, 2]) instanceof Object) // true (we want this to be false so it's not serialized as an object)

console.log(Object.getPrototypeOf({}) == Object.prototype) // true
console.log(Object.getPrototypeOf([]) == Array.prototype) // true
console.log(Object.getPrototypeOf(new Set([1, 2])) == Object.prototype) // false (this won't be serialized as an object)
promer94 commented 2 weeks ago

Thanks for your awesome reproduction ❤️

The instanceof works because edge-runtime uses proxy to patch the behavior https://github.com/vercel/edge-runtime/blob/d6cb77fba517c2e55b9c0353b43f5f2d50c466a2/packages/vm/src/edge-vm.ts#L120

I'm still unclear why .constructor doesn't work in the edge runtime, though.

the reason why the constructor of [] is not globalThis.Array is because https://github.com/vercel/next.js/issues/38184#issuecomment-1172212418