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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose structuredClone #3414

Open
wants to merge 5 commits into
base: master
from

Conversation

@surma
Copy link

commented Jan 27, 2018

This is in connection to #793, to make the discussion a bit more tangible.

Do not merge. This still requires interest/agreement from other browser vendors.

This is my first time touching the HTML spec and also my first time writing a spec section from scratch. I probably didn鈥檛 do everything right, so please be thorough in your review.

WPT PR here: web-platform-tests/wpt#9218


馃挜 Error: Wattsi server error 馃挜

PR Preview failed to build. (Last tried on Dec 14, 2018, 6:49 PM UTC).

More

馃敆 Related URL

Parsing MDN data...
Parsing caniuse.com data...
Processing caniuse.com data...
Parsing...
Generating HTML variant...
Saving index-html
Splitting...
Saving index.html
Saving introduction.html
Saving infrastructure.html
Saving common-microsyntaxes.html
Saving urls-and-fetching.html
Saving common-dom-interfaces.html
Saving structured-data.html
Saving dom.html
Saving semantics.html
Saving sections.html
Saving grouping-content.html
Saving text-level-semantics.html
Saving links.html
Saving edits.html
Saving embedded-content.html
Saving images.html
Saving iframe-embed-object.html
Saving media.html
Saving image-maps.html
Saving embedded-content-other.html
Saving tables.html
Saving forms.html
Saving input.html
Saving form-elements.html
Saving form-control-infrastructure.html
Saving interactive-elements.html
Saving scripting.html
Saving canvas.html
Saving custom-elements.html
Saving semantics-other.html
Saving microdata.html
Saving interaction.html
Saving dnd.html
Saving browsers.html
Saving window-object.html
Saving origin.html
Saving history.html
Saving browsing-the-web.html
Saving offline.html
Saving webappapis.html
Saving dynamic-markup-insertion.html
Saving timers-and-user-prompts.html
Saving system-state.html
Saving imagebitmap-and-animations.html
Saving comms.html
Saving server-sent-events.html
Saving web-sockets.html
Saving web-messaging.html
Saving workers.html
Saving webstorage.html
Saving syntax.html
Saving parsing.html
Saving named-characters.html
Saving xhtml.html
Saving rendering.html
Saving obsolete.html
Saving iana.html
Saving indices.html
Saving references.html
Saving acknowledgements.html
Generating DEV variant...
Warning: Could not find ID selector-read-only for annotation that uses URLs:
   https://caniuse.com/#feat=css-read-only-write
Splitting...
Saving index.html
Saving introduction.html
Saving infrastructure.html
Saving common-microsyntaxes.html
Saving urls-and-fetching.html
Saving common-dom-interfaces.html
Saving dom.html
Saving semantics.html
Saving sections.html
Saving grouping-content.html
Saving text-level-semantics.html
Saving links.html
Saving edits.html
Saving embedded-content.html
Saving images.html
Saving iframe-embed-object.html
Saving media.html
Saving image-maps.html
Saving embedded-content-other.html
Saving tables.html
Saving forms.html
Saving input.html
Saving form-elements.html
Saving form-control-infrastructure.html
Saving interactive-elements.html
Saving scripting.html
Saving canvas.html
Saving custom-elements.html
Saving semantics-other.html
Saving microdata.html
Saving interaction.html
Saving dnd.html
Saving browsers.html
Saving window-object.html
Saving origin.html
Saving history.html
Saving browsing-the-web.html
Saving offline.html
Saving webappapis.html
Saving dynamic-markup-insertion.html
Saving timers-and-user-prompts.html
Saving system-state.html
Saving imagebitmap-and-animations.html
Saving comms.html
Saving server-sent-events.html
Saving web-sockets.html
Saving web-messaging.html
Saving workers.html
Saving webstorage.html
Saving syntax.html
Saving named-characters.html
Saving xhtml.html
Saving obsolete.html
Saving iana.html
Saving indices.html
Saving references.html
Saving acknowledgements.html
Generating SNAP variant...
Error: Multiple elements found with ID "structured-cloning"
Error count: 1
Saving index-snap



If the error is not surfaced properly, please file an issue.

source Outdated
@@ -90499,6 +90503,42 @@ document.body.appendChild(frame)</pre>
</div>


<h3 id="structuredcloning">Structured Cloning</h3>

This comment has been minimized.

Copy link
@surma

surma Jan 27, 2018

Author

After seeing the diffs... this probably doesn鈥檛 warrant a new <h3>, does it?

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

Fine with me. This section seems to be on the same magnitude as Base64 utilities, which is the <h3> right before this one.

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

Sentence case here too.

source Outdated

<p>The <code data-x="dom-structured-clone">structuredClone(<var>data</var>)</code>
method must throw an <span>"<code>DataCloneError</code>"</span> <code>DOMException</code>
if <var>data</var> is a <span data-x="serializable objects">non-serializable object</span>.</p>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

This is unnecessary in the normative section. Structured(De)Serialize throw errors themselves already, and the ? syntax should make it obvious that the underlying algorithms may throw exceptions.

This comment has been minimized.

Copy link
@surma

surma Jan 28, 2018

Author

Ah, thank you! I was looking for an explanation what exactly ? means.

source Outdated
method, when invoked, must run the following steps:</p>

<ol>
<li><p>Let <var>serialized</var> be ? <span>StructuredSerializeInternal</span>(<var>value</var>, false).</p></li>

This comment has been minimized.

Copy link
@TimothyGu
source Outdated

<ol>
<li><p>Let <var>serialized</var> be ? <span>StructuredSerializeInternal</span>(<var>value</var>, false).</p></li>
<li><p>Return ? <span>StructuredDeserialize</span>(<var>serialized</var>, []).</p></li>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

This looks wrong. StructuredDeserialize takes a serialized Record serialized, a Realm record, and an optional map memory. You probably want

  1. Let realm be the current Realm Record.
  2. Return ? StructuredDeserialize(serialized, realm).

instead.

source Outdated

<p>Takes the input data and returns a deep-copy by performing the structural clone algorithm.</p>

<p>Throws an <span>"<code>DataCloneError</code>"</span> <code>DOMException</code>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

s/ an / a /

source Outdated
<p>Takes the input data and returns a deep-copy by performing the structural clone algorithm.</p>

<p>Throws an <span>"<code>DataCloneError</code>"</span> <code>DOMException</code>
exception if any part of the input data is not <span data-x="serializable objects">serializable.</span></p>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

This reads a bit odd: "throws a "DataCloneError" DOMException exception" I'm fine with removing the second "exception". Also, I think the general style is to have the period after the link (</span>. instead of .</span>).

source Outdated
@@ -90499,6 +90503,42 @@ document.body.appendChild(frame)</pre>
</div>


<h3 id="structuredcloning">Structured Cloning</h3>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

Fine with me. This section seems to be on the same magnitude as Base64 utilities, which is the <h3> right before this one.

@annevk
Copy link
Member

left a comment

Some additional nits from me. Thanks for taking this on. If you can convince the powers that be at Chrome (and add some tests) we can move forward with this. (Potentially after adding transferables.)

source Outdated
@@ -90401,6 +90401,10 @@ interface mixin <dfn>WindowOrWorkerGlobalScope</dfn> {
// ImageBitmap
Promise&lt;<span>ImageBitmap</span>&gt; <span data-x="dom-createImageBitmap">createImageBitmap</span>(<span>ImageBitmapSource</span> image, optional <span>ImageBitmapOptions</span> options);
Promise&lt;<span>ImageBitmap</span>&gt; <span data-x="dom-createImageBitmap">createImageBitmap</span>(<span>ImageBitmapSource</span> image, long sx, long sy, long sw, long sh, optional <span>ImageBitmapOptions</span> options);

// Structured Cloning

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

Sentence case.

source Outdated
@@ -90401,6 +90401,10 @@ interface mixin <dfn>WindowOrWorkerGlobalScope</dfn> {
// ImageBitmap
Promise&lt;<span>ImageBitmap</span>&gt; <span data-x="dom-createImageBitmap">createImageBitmap</span>(<span>ImageBitmapSource</span> image, optional <span>ImageBitmapOptions</span> options);
Promise&lt;<span>ImageBitmap</span>&gt; <span data-x="dom-createImageBitmap">createImageBitmap</span>(<span>ImageBitmapSource</span> image, long sx, long sy, long sw, long sh, optional <span>ImageBitmapOptions</span> options);

// Structured Cloning
any <span data-x="dom-structured-clone">structuredClone</span>(any obj);

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

I prefer "value" since you can clone any ECMAScript value.

source Outdated
@@ -90499,6 +90503,42 @@ document.body.appendChild(frame)</pre>
</div>


<h3 id="structuredcloning">Structured Cloning</h3>

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

Sentence case here too.

source Outdated
<p>The <code subdfn data-x="dom-structured-clone">structuredClone()</code> method allows developers to copy a value.</p>

<dl class="domintro">

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

No newline.

source Outdated
<dl class="domintro">

<dt><var>result</var> = self . <code data-x="dom-structured-clone">structuredClone</code>( <var>data</var> )</dt>

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

No newline.

source Outdated
<dt><var>result</var> = self . <code data-x="dom-structured-clone">structuredClone</code>( <var>data</var> )</dt>

<dd>

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

No newline.

source Outdated

<p>Throws an <span>"<code>DataCloneError</code>"</span> <code>DOMException</code>
exception if any part of the input data is not <span data-x="serializable objects">serializable.</span></p>

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

No newline.

source Outdated
exception if any part of the input data is not <span data-x="serializable objects">serializable.</span></p>

</dd>

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

No newline.

source Outdated
method must throw an <span>"<code>DataCloneError</code>"</span> <code>DOMException</code>
if <var>data</var> is a <span data-x="serializable objects">non-serializable object</span>.</p>

<p>The <dfn data-x="dom-structured-clone"><code id="dom-structured-clone">structuredClone(<var>data</var>)</code></dfn>

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

You need to name the argument consistently with the IDL. (And also make sure you consistently use it within the method description. 馃槈)

source Outdated
method, when invoked, must run the following steps:</p>

<ol>
<li><p>Let <var>serialized</var> be ? <span>StructuredSerialize</span>(<var>value</var>, false).</p></li>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Jan 28, 2018

Member

StructuredSerialize only takes one argument.

Other than the comment, the spec text LGTM. However, this will need tests (and of course, browser support).

source Outdated

<div w-nodev>

<p>The <dfn data-x="dom-structured-clone"><code id="dom-structured-clone">structuredClone(<var>value</var>, <var>transferList</var>)</code></dfn> method, when invoked, must run the following steps:</p>

This comment has been minimized.

Copy link
@surma

surma Jan 28, 2018

Author

I鈥檓 assuming WebIDL takes care of handling the default value? No need to duplicate it here?

This comment has been minimized.

Copy link
@annevk

annevk Jan 28, 2018

Member

Correct.

@surma surma referenced this pull request Jan 28, 2018
@surma

This comment has been minimized.

Copy link
Author

commented Jan 28, 2018

@domenic

This comment has been minimized.

Copy link
Member

commented Jan 28, 2018

I think this section is misplaced. It should be part of https://html.spec.whatwg.org/multipage/structured-data.html#safe-passing-of-structured-data, probably a new sub-section at the end.

For the dev edition, the contents of this new subsection (not including its heading) should be the only content of the "Safe passing of structured data" section.

@mathiasbynens
Copy link
Member

left a comment

Thanks for getting the ball rolling on this, Surma! 馃憤馃徎

source Outdated
@@ -90499,6 +90503,37 @@ document.body.appendChild(frame)</pre>
</div>


<h3 id="structuredcloning">Structured cloning</h3>

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Jan 28, 2018

Member

id="structured-cloning" for consistency with other headings.

source Outdated
<dt><var>result</var> = self . <code data-x="dom-structured-clone">structuredClone</code>( <var>value</var>
[, <var>transferList</var> ] )</dt>
<dd>
<p>Takes the input value and returns a deep-copy by performing the structural clone algorithm. <span>Transferable

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Jan 28, 2018

Member

Nit: s/deep-copy/deep copy/

@surma

This comment has been minimized.

Copy link
Author

commented Jan 30, 2018

@domenic I hope I understood your feedback right. PTAL

@tobireif

This comment has been minimized.

Copy link

commented Feb 1, 2018

As a web developer / JS API user I don't care so much about how the object gets cloned (whether the "structural clone algorithm" is used or a different way of deep cloning). The name of the method doesn't have to reflect the choice of algorithm used by the implementation.

Could the method be called clone() instead of structuredClone() ? or eg deepClone() or completeClone() or completeCopy() -
or perhaps just simply copy() ? (I haven't gone through all existing names)

@surma

This comment has been minimized.

Copy link
Author

commented Feb 1, 2018

The name of the method doesn't have to reflect the choice of algorithm

I disagree. The name should to convey what kind of clone the result is. The method structuredClone give you a, well, structured clone. Meaning it directly implies what will be cloned and what will not be cloned (e.g. methods and prototypes are not cloned).

A name like deepClone or completeCopy could set expectations with the developer that are not being fulfilled. That鈥檚 how we end up with misnomers on the platform.

@tobireif

This comment has been minimized.

Copy link

commented Feb 1, 2018

I see, thanks for your reply.

Perhaps we do need a method deepClone() or completeCopy()?

@surma

This comment has been minimized.

Copy link
Author

commented Feb 1, 2018

Yeah, we have talked about that. But cloning functions is not trivial. Please continue discussing that matter in #793 :)

@annevk

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

To reiterate, my main problem with this proposal is that it doesn't expose the full underlying primitive, but rather a subset, and it's unclear how to extend it to expose it fully. The primitive we have is one that takes a value and a transfer list, and returns a value and a transfer list. And the transfer list can be independent from the value. This API just drops the return transfer list on the floor.

@surma

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

Thanks for re-iterating, I didn鈥檛 get that before. That鈥檚 fair, I guess. I don鈥檛 see a problem with changing the returned value to {cloned, transferredValues}.

@domenic

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

I'm not sure that primitive is terribly important. I think it's more for handling cases where for whatever reason you don't put the transferred values inside the cloned data, and for spec convenience in not making you dig through to find those values, but IMO such cases are not an important part of the primitive...

@surma

This comment has been minimized.

Copy link
Author

commented Feb 16, 2018

I agree, but I鈥檓 also not sure if we are thinking of everything that could be needed in the future. I鈥檒l leave that decision up to you all, I don鈥檛 have very strong feelings either way, although in terms of developer ergonomics and arguably the current 99% use-case, returning just the clone seems more desirable.

@jeremyroman

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

FWIW, +1 for just returning the deserialized values. It'd be really easy for a caller to get if they wanted, be serializing {data, transferList} themselves. Currently [[TransferredValues]] is only exposed insofar as MessagePort objects are extracted from there and separately exposed. As a result, Blink doesn't actually keep the original transfer list in order at present (we have separate lists for ports, buffers, etc.).

@surma

This comment has been minimized.

Copy link
Author

commented May 18, 2018

The original PR for tests got side-tracked and the actual PR for adding tests for structuredClone() now lives in web-platform-tests/wpt#11069.

@surma

This comment has been minimized.

Copy link
Author

commented May 21, 2018

Just wondering: I am not sure what the result of the conversation so far is. Have we reached some sort of consensus weather transferList should be exposed by structuredClone()? What are we missing to qualify for 鈥渋mplementer interest鈥?

@domenic

This comment has been minimized.

Copy link
Member

commented May 21, 2018

I don't know about consensus, but I feel it should definitely be there, and my read of @annevk was that he didn't feel too strongly against it...

What we're missing in terms of implementer interest is outlined in https://whatwg.org/working-mode#additions. So far I don't see any browser engineers saying "we would like to implement this soon" on this thread.

@annevk

This comment has been minimized.

Copy link
Member

commented May 22, 2018

I suspect Firefox would adopt this pretty quickly, but it'd be good if someone could go over #793 (comment) and work through @wanderview's concern.

@IamManchanda

This comment has been minimized.

Copy link

commented Jul 27, 2018

Can you guys have a look once at tc39/proposals#150

Can we guys discuss?

@GrosSacASac

This comment has been minimized.

Copy link

commented Nov 13, 2018

It is sad that there is not more interest shown, considering that the top 3 npm packages for cloning have over 100k weekly downloads

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can鈥檛 perform that action at this time.