-
Notifications
You must be signed in to change notification settings - Fork 161
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce the observable array type (legacy platform object based) #836
Conversation
4149e1e
to
9cb995b
Compare
I'm not 100% happy with how the hooks have turned out here, both for spec authors and for web developers using the constructor. Here are some potential models:
I'm not sure what would be best. The |
|
||
There is no way to represent a constant observable array value in IDL. | ||
|
||
The [=type name=] of a frozen array type is the concatenation of the type name for |T| and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The [=type name=] of a frozen array type is the concatenation of the type name for |T| and the | |
The [=type name=] of an observable array type is the concatenation of the type name for |T| and the |
Instead of the usual conversion algorithms, observable array types have special handling as part of | ||
the [=attribute getter=] and [=attribute setter=] algorithms. | ||
|
||
|
||
<h5 id="create-frozen-array-from-iterable" dfn>Creating a frozen array from an iterable</h5> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several instances of "frozen" still present in this section. Also this ends up removing the actual "create a frozen array from an iterable" section, which I suspect wasn't intended.
Strong agree that we need at least a 4-hook model. 2 hooks are unsound; there's nothing theoretically preventing an ObservableArray from allowing 1 hook is sound, but annoying; most of the time I'll just be validating the one value being set, and having to specify that as iterating the list and revalidating everything would be both annoying and less clear in my intent. I don't have an opinion on which of the 4/6 hook options to go for, but I lean slightly toward update/delete pair. |
Is it necessary to make these types constructible? This doesn't seem necessary for the Some thoughts about hooks. Do the |
It's not strictly necessary, but it's good API design, per https://w3ctag.github.io/design-principles/#constructors. For example, it'd be sad if we introduced a second use of However, the approach I'm currently working on (I hope to have a PR up today), per #796 (comment), does remove the need for a constructor, and thus lets us sidestep a lot of these questions :). I'm optimistic.
That's a great point. Leaving aside the JS-constructed question for a moment, which hopefully gets oviated by the Proxy-based approach, and talking purely about spec authorship: I think the main benefit of the after hooks is that when they run, your array is entirely consistent. So e.g. if you wanted to express "set the UA's X to the observable array contents", you could do that pretty easily in an after hook, whereas in a before hook, you would have to communicate that you want the current observable array contents, with the new item inserted at the right place, to be used to update the UA's X.
I think having the spec author provide getter/setter algorithms, as is currently done for indexed getters/setters in Web IDL, doesn't work when author mutation is allowed. The spec-author-provided getter, in particular, can't be sure what to get, if there have been author mutations in the meantime. Or, well, it can, but doing so would require adding spec text to the setter to track all such mutations. This approach attempts to move all that bookkeeping into the Web IDL spec, so that the spec author can instead use the always-updated "backing observable array contents" list, plus the hooks to react to changes to that list. |
It's likely that browser engine implementations would want to actually implement using index getters and setters against a custom backing store. It's better if spec concepts can map more closely to how things would be implemented (though of course not required, particularly if it hurts spec clarity). Is there a way to make this easy to use for spec authors without the dual hooks? I think the idea I suggested of passing a function to the hook that does the raw operation on the backing store would probably be not much harder for specs, and might make the algorithms more clear, since they would not be split into two parts. One other thought: many Array operations end up doing quite a few insertions, deletions or updates as specified. It would be desirable to batch updates in such cases. Perhaps this would fall out naturally if any observables like this only really need to update at next microtask. But otherwise, the way Array operations are defined Maes this hard to do. |
How would that look? I have literally no idea how to create a function and pass it around in IDL-ese, particularly one that operates in the IDL-value space rather than in JS-space. |
In general I think the lesson we learned from FrozenArray (see various issues in this repo) is that we need to optimize very strongly for spec-author convenience for these sorts of types. I'd like to get an ideal model for spec authors (and web developers!) as the highest priority, then worry about implementation mapping. |
@tabatkins I guess you'd have to pass an interface, not a function, if IDL has no way to represent a function. (I think there are cases where functions can be passed in, e.g. rAF or setTimeout, but none as callback parameters). @domenic It makes sense to design something that's hard for spec authors to get wrong. I do think classic index getters and setters haven't created much trouble for spec authors, so not sure why we need a different and more complex model just to give the setter the ability to reject the change (which I think is the only material difference here; all the Array functions would work via index access and |
The big material difference here is that, unlike indexed getters and setters, here web developers can mutate the array contents. As I said above,
|
But all mutation would go through the index setter. That’s how mutating Array methods are defined. I don’t think there is a way to mutate that bypasses it. |
Correct. So the spec author would need to track all such mutations and sync them back to their backing collection, which is not as convenient for cases like adoptedStyleSheets, where you want to instead replace the current spec text's
with
If you had to do this with the indexed setter, you'd have to convert each set into a manipulation of a backing list, and then have a sentence like the above that refers to the backing list. Not a big deal, but a sharp edge. (BTW, the examples of indexed setters I can find in the platform so far are https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#dom-htmloptionscollection-setter and https://svgwg.org/svg2-draft/types.html#ListInterfaces. It's interesting to contemplate how well they make use of the existing hooks.) |
After a bit more work hacking on the proxy variant (to fix the issues noted in #796 (comment)), I'm coming around to @othermaciej's point of view, that we make the hooks behave like indexed setters (and the no-longer-in-the-spec indexed deleters). The reasoning is a bit subtle. It turns out maintaining an accurate "observable array backing contents" list is not really possible due to the potential for holes in the array. For example, consider JS code such as // Creates 3 holes at indices 0, 1, 2
building.employees.length = 3;
building.employees = [new Employee("A"), new Employee("B")];
// Creates a hole at index 0
delete building.employees[0]; One might ask, can we disallow such code? It turns out no, at least not if we want array methods to work, because array methods often create such holes "temporarily". E.g. // Create a temporary hole
delete building.employees[building.employees.length - 1];
// Truncate the array, removing the temporary hole
building.employees.length = building.employees.length - 1; and there's no way to distinguish these kinds of "temporary" holes (which we want to allow) from the more long-term ones created by the above snippets. Given this, the "backing observable array contents" I was trying to preserve will either:
So I'm thinking that a simple indexed setter/deleter-type infrastructure will be good enough instead, since the perceived benefits of the proposal in this PR don't really materialize. |
Interesting. I would have expected In any case, I think it's possible to implement I think that would also allow That does mean we still need a custom [[Delete]]. Not sure whether that's an issue in practice yet; as I said I won't be able to read through everything carefully before Monday. |
(Ok, technically for Arrays it could just set I have no suggestions for the broader problem of how to deal with holes. |
Let me close this, as #840 is much better in my opinion, and is better-reviewed. |
Part of #796.
Preview | Diff