urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.63k stars 451 forks source link

fix(core): Fix JSON stringification in Vercel Edge runtime #3453

Closed SoraKumo001 closed 9 months ago

SoraKumo001 commented 10 months ago

Summary

In EdgeRuntime in Next.js, the constructor of an object does not match Object. Therefore, it is necessary to determine if it inherits from Object.

Set of changes

The following changes are made to the determination of the part that creates the keys of variables.

x.constructor !== Object    ↓ !(x.constructor instanceof Object)

changeset-bot[bot] commented 10 months ago

🦋 Changeset detected

Latest commit: 8ef386cf1f012301959eab1c270d78f844df4b3f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ---------- | ----- | | @urql/core | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

kitten commented 10 months ago

Thanks for taking the time to report this 🙌 I'll have to do some testing on Vercel Edge Runtimes and see what's going on. I'm not sure if this also affects e.g. Cloudflare's Workers, but it'd be good to find out.

I'm not quite sure the fix here preserves the intention of the code, since it might break the following cases:

// These shouldn't result in `true`
(new (class Test {}) instanceof Object);
(new Map() instanceof Object);
(new Set() instanceof Object);
((new Request('http://0.0.0.0')) instanceof Object);

In other words, we want to detect non-serialisable objects, with some exceptions. I had another approach in mind via Object.getPrototypeOf, but I'll have to look into some options to see whether this actually also works

SoraKumo001 commented 10 months ago

@kitten

Replaced the decision with the following constructor.name !== 'Object'

const Objects = [
  new (class Test {})(),
  new Map(),
  new Set(),
  new Request('http://0.0.0.0'),
  {},
  { test: 'abc' },
];

Objects.forEach((value) => {
  console.log(value.constructor.name === 'Object');
});
false
false
false
false
true
true
SoraKumo001 commented 10 months ago

The following modifications have been made to allow for a strict determination. Object.getPrototypeOf(x).constructor.name !== 'Object'

test.js

const Objects = [
  new (class Test {})(),
  new Map(),
  new Set(),
  new Request('http://0.0.0.0'),
  function () {},
  true,
  'abc',
  123,
  Symbol('a'),
  [],
  new Date(),
  new RegExp(''),
  new Error(),
  new Promise(() => {}),
  new (class extends Object {})(),
  JSON.parse('{}'),
  {},
  { test: 'abc' },
  { constructor: { name: 'Object2' } },
];

Objects.forEach((value) => {
  try {
    console.log(
      Object.getPrototypeOf(value).constructor.name === 'Object',
      Object.getPrototypeOf(value).constructor.name
    );
  } catch (e) {
    console.log(e);
  }
});

node test.js

false Test
false Map
false Set
false Request
false Function
false Boolean
false String
false Number
false Symbol
false Array
false Date
false RegExp
false Error
false Promise
false
true Object
true Object
true Object
true Object

edge-runtime test.js

false 'Test'
false 'Map'
false 'Set'
false 'Request'
false 'Function'
false 'Boolean'
false 'String'
false 'Number'
false 'Symbol'
false 'Array'
false 'Date'
false 'RegExp'
false 'Error'
false 'Promise'
false ''
true 'Object'
true 'Object'
true 'Object'
true 'Object'
SoraKumo001 commented 10 months ago

Fixed to compare the constructor itself. Object.getPrototypeOf(value).constructor !== Object.prototype.constructor

test.js

const Objects = [
  new (class Test {})(),
  new Map(),
  new Set(),
  new Request('http://0.0.0.0'),
  function () {},
  true,
  'abc',
  123,
  Symbol('a'),
  [],
  new Date(),
  new RegExp(''),
  new Error(),
  new Promise(() => {}),
  new (class extends Object {})(),
  JSON.parse('{}'),
  {},
  { test: 'abc' },
  { constructor: { name: 'Object2' } },
];

Objects.forEach((value) => {
  try {
    console.log(
      Object.getPrototypeOf(value).constructor === Object.prototype.constructor,
      Object.getPrototypeOf(value).constructor.name
    );
  } catch (e) {
    console.log(e);
  }
});

node test.js

false Test
false Map
false Set
false Request
false Function
false Boolean
false String
false Number
false Symbol
false Array
false Date
false RegExp
false Error
false Promise
false
true Object
true Object
true Object
true Object

edge-runtime test.js

false 'Test'
false 'Map'
false 'Set'
false 'Request'
false 'Function'
false 'Boolean'
false 'String'
false 'Number'
false 'Symbol'
false 'Array'
false 'Date'
false 'RegExp'
false 'Error'
false 'Promise'
false ''
true 'Object'
true 'Object'
true 'Object'
true 'Object'
kitten commented 9 months ago

Amazing research! 🙌 I double checked some of this, but don't think it'll need any changes. Just added the tests I had locally and a changeset for our changelog ❤️ Thank you for all the help! 💯