node icon indicating copy to clipboard operation
node copied to clipboard

`structuredClone` Serializing a non-serializable platform object succeeds

Open avivkeller opened this issue 1 year ago • 2 comments

In Node.js, the following snippet succeeds, whereas, in the browser, it throws a DataCloneError:

structuredClone(new Response());

This is causing the following WPT to fail:

structuredCloneBatteryOfTests.push({
  description: 'Serializing a non-serializable platform object fails',
  async f(runner, t) {
    const request = new Response();
    await promise_rejects_dom(
      t,
      "DataCloneError",
      runner.structuredClone(request)
    );
  }
});

avivkeller avatar Sep 25 '24 18:09 avivkeller

A JavaScript value value is a platform object if Type(value) is Object and if value has a [[PrimaryInterface]] internal slot.

The primary interface of a platform object is the value of the object’s [[PrimaryInterface]] internal slot, which is the most-derived interface that it implements.

Request's webidl declaration does not extend anything, therefore it's a platform object.

Hopefully this may help someone.

KhafraDev avatar Sep 26 '24 18:09 KhafraDev

@nodejs/web-standards @joyeecheung

KhafraDev avatar Sep 26 '24 18:09 KhafraDev

As you've pointed out, the behavior where structuredClone(new Response()) succeeds in Node.js but throws a DataCloneError in the browser is indeed intriguing. This difference stems from the fact that the Response object is classified as a platform object, which, by design, is not serializable. In the browser environment, trying to clone such objects will rightfully lead to an error, aligning with the Web Platform's handling of non-serializable entities.

when using structuredClone, we should expect it to fail for these types of objects, as seen in the browser.

Web Platform Tests (WPT) reflect this behavior accurately, consider modifying the test cases. Here's a potential approach for your tests:

structuredCloneBatteryOfTests.push({ description: 'Serializing a non-serializable platform object fails', async f(runner, t) { const request = new Response(); await promise_rejects_dom( t, "DataCloneError", runner.structuredClone(request) ); } }); Monitoring Pull Request #55178: The pull request #55178, which focuses on decorating undici classes as platform interfaces, may provide helpful adjustments to address this inconsistency. Keeping an eye on the developments of this PR could lead to insights that may resolve the underlying issue.

I appreciate your efforts in bringing this issue to light, and I look forward to further discussions as we work towards a resolution.

sOnU1002 avatar Oct 01 '24 10:10 sOnU1002

I am a bit curious how undici will adopt this to address this issue. Don't get me wrong, it will address the issue, but the process will probably be like:

  1. release that PR
  2. land a new commit on undici to mark all classes uncloneable, which might be semver-major
  3. node waits for that undici release and bump the deps

The whole thing could be dragged a bit long, and I am thinking should we consider the fast win with a TODO to drop it once the above process is settled?

jazelly avatar Oct 04 '24 02:10 jazelly

It won't be a semver-major issue, changes that are made for spec-compatibility are usually considered bug fixes. This bug was only caught in a WPT - the behavior is far too niche to justify the performance overhead of constructing every Response, Request, FormData, Headers, etc.

KhafraDev avatar Oct 04 '24 03:10 KhafraDev

@jazelly just confirming the Unidici PR fixed this, and it'll be good-to-go when the next version of Undici releases?

avivkeller avatar Oct 18 '24 00:10 avivkeller

Yes, once that's released and bumped in node, this should be resolved.

jazelly avatar Oct 18 '24 00:10 jazelly

I'm gonna close this, as while the latest Node.js doesn't have the Undici needed, it's no longer actionable as an issue. Thx!

avivkeller avatar Oct 18 '24 00:10 avivkeller