Re: [heycam/webidl] Introduce the observable array type (proxy-based) (#840)

domenic commented on this pull request.

Thanks for taking a look, and sorry for letting your reply sit!

> Presumably the interface the property is defined on would need to maintain a mirror of the list that it syncs using the delete/set hooks. [...]  I'm not 100% sure whether there are use cases for not maintaining a mirrored list.

I've been going back and forth on this. Now that we're trying to maintain the no-holes invariant, we could maintain the mirror list in the IDL spec and let spec authors refer to it without much trouble.

I went with the current approach based on @othermaciej's comments around https://github.com/heycam/webidl/pull/836#issuecomment-579387799, which indicated a desire to align with indexed setters/getters. Notably classes that use those must sync to any mirrors manually, if they want to, and some classes that use indexed setters don't seem to want such a mirror. (Example: [HTMLOptionsCollection](https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#dom-htmloptionscollection-setter).)

I attempted to prototype what would be needed for the `adoptedStyleSheets` case in https://github.com/WICG/construct-stylesheets/pull/117, but it ended up not being super-helpful, because it's building on non-rigorous foundations. (Apparently nothing in CSS actually dictates what style sheets are applied to a page; see discussion in https://github.com/WICG/construct-stylesheets/issues/118.) Probably the best step is to try to rigorize those further and see if an IDL-maintained mirror list would be helpful or not. I suspect it would be.

> One question I do have, just in terms of which bits here are inherent to the proposal vs which are accidental. What would the observable behavior changes be, if any, if the proxy had two arrays backing it: one the proxy target and one for storing all the indexed props and length state (and possibly all props?). It'd require a lot more hooks (e.g. getOwnPropertyDescriptor), but assuming we implemented those?
>
> Or put another way, is the proposal black-box-identical to [...]. I _think_ that except for the cases when T is a dictionary or sequence type these are all black-box equivalent.

With the no-holes prohibition in place, I agree it should be black-box identical. (Without that the backing list gets awkward and needs to have holes in it.) I had some version of this locally before abandoning it for the current one, which reuses more machinery.

The fact that you caught the dictionary/sequence type problem is a little worrying that there might be other things we missed, but disallowing those for now seems good...


> @@ -6775,6 +6782,122 @@ The [=type name=] of a frozen array
 type is the concatenation of the type name for |T| and the string
 "<code>Array</code>".
 
+<h4 id="idl-observable-array" interface lt="ObservableArrayT|ObservableArray&lt;T&gt;">Observable array types — ObservableArray&lt;|T|&gt;</h4>
+
+An <dfn id="dfn-observable-array-type" export>observable array type</dfn> is a parametrized type
+whose values are references to objects of type |T|. The contents of the array can be mutated, both

I guess I'm not really sure what "whose values are" is supposed to summarize here. This is the general section, not the ES binding section, so this statement is a peer to things like "The `sequence<T>` type is a parameterized type whose values are (possibly zero-length) lists of values of type T" or "A promise type is a parameterized type whose values are references to objects that “is used as a place holder for the eventual results of a deferred (and possibly asynchronous) computation result of an asynchronous operation”." It doesn't seem quite right to reference proxies at this point.

> +
+Similar to [=sequence types=] and [=frozen array types=], observable array types wrap around
+ECMAScript array types, imposing additional semantics on their usage.
+
+Observable array types must only be used as the type of [=regular attributes=].
+
+<p class="note">We could also allow them as operation return values with a bit more work. Please 
+<a href="https://github.com/heycam/webidl/issues/new?title=Enhancement%20request%20for%20observable%20array%20return%20values">file an issue</a>
+if you would like to use them as such.</p>
+
+For an attribute whose type is an observable array type, specification authors can specify a series
+algorithms:
+
+*   <dfn for="observable array attribute">set an indexed value</dfn>, which accepts an IDL value
+    that is about to be set in the observable array, and the index at which it is being set;
+*   <dfn for="observable array attribute">delete an indexed value</dfn>, which accepts an IDL value

It's not clear what the right stuff to pass in is; I'm kind of just making up hooks as we go along. https://github.com/WICG/construct-stylesheets/pull/117 uses it in a handwavey way. I guess I'll try to make that less handwavey (as described above) and see what's still wanted.

> +
+    The behavior of the attribute could be defined like so:
+
+    <blockquote>
+        The [=observable array attribute/set an indexed value=] algorithm for
+        <code class="idl">Building</code>'s <code class="idl">employees</code> attribute, given
+        |employee| and |index|, is:
+
+        1.  If |employee| is not allowed to enter the building today, then throw an
+            "{{NotAllowedError}}" {{DOMException}}.
+        1.  If |index| is greater than 200, then throw a "{{QuotaExceededError}}" {{DOMException}}.
+        1.  Put |employee| to work!
+
+        The [=observable array attribute/delete an indexed value=] algorithm for
+        <code class="idl">Building</code>'s <code class="idl">employees</code> attribute, given
+        |employee|, is:

It's not used, so it's omitted, JavaScript-style... I guess that could be confusing.

> +        1.  If [=!=] [$IsAccessorDescriptor$](|descriptor|) is <emu-val>true</emu-val>, then return
+            <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Configurable]] field and |descriptor|.\[[Configurable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Enumerable]] field and |descriptor|.\[[Enumerable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Writable]] field and |descriptor|.\[[Writable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If [=!=] [$ToUint32$](|P|) > |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |descriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.
+        1.  Let |existingIDLValue| be the result of [=converted to an IDL value|converting=]
+            |existingDescriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Assert: the above step never throws an exception, since we already went through the

+1 to disallowing those; returning a new object every time would be terrible. We should probably disallow records for the same reason, right?

> +    1.  Let |handler| be the <emu-val>this</emu-val> value.
+    1.  If |P| [=is an array index=], then:
+        1.  Let |oldLenDesc| be [=!=] [$OrdinaryGetOwnProperty$](|O|, "length").
+        1.  Assert: [=!=] [$IsDataDescriptor$](|oldLenDesc|) is <emu-val>true</emu-val>.
+        1.  Let |oldLen| be |oldLenDesc|.\[[Value]].
+        1.  If [=!=] [$ToUint32$](|P|) ≠ |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |existingDescriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Assert: the above step never throws an exception, since we already went through the
+            conversions in
+            <a href="#es-observable-array-defineProperty">the <code>defineProperty</code> trap</a>.
+        1.  Perform the algorithm steps given by |handler|.\[[DeleteAlgorithm]], given
+            |idlValue| and |P|.
+        1.  Perform [=!=] |O|.\[[Delete]](|O|, |P|).

That would recurse into this delete handler.

> +        1.  If |descriptorObj|.\[[Value]] is present and |descriptorObj|.\[[Value]] > |oldLen|,
+            return <emu-val>false</emu-val>.
+    1. Else if |P| [=is an array index=], then:
+        1.  If [=!=] [$IsAccessorDescriptor$](|descriptor|) is <emu-val>true</emu-val>, then return
+            <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Configurable]] field and |descriptor|.\[[Configurable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Enumerable]] field and |descriptor|.\[[Enumerable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If |descriptor| has a \[[Writable]] field and |descriptor|.\[[Writable]] is
+            <emu-val>false</emu-val>, return <emu-val>false</emu-val>.
+        1.  If [=!=] [$ToUint32$](|P|) > |oldLen|, return <emu-val>false</emu-val>.
+        1.  Let |idlValue| be the result of [=converted to an IDL value|converting=]
+            |descriptor|.\[[Value]] to the type given by |handler|.\[[Type]].
+        1.  Let |existingDescriptor| be [=!=] [$OrdinaryGetOwnProperty$](|O|, |P|).
+        1.  Assert: |existingDescriptor| is not <emu-val>undefined</emu-val>.

Good catch, in that case we shouldn't be running delete, and in general it doesn't seem to be handled.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/840#pullrequestreview-361361046