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’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

Closed
wants to merge 1 commit into from

Conversation

domenic
Copy link
Member

@domenic domenic commented Jan 24, 2020

Part of #796.


Preview | Diff

@domenic
Copy link
Member Author

domenic commented Jan 24, 2020

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:

  • Two hooks, before(value, index) and after(value, index), which get undefined for deletions.
  • 6 hooks, {before,after}Add(newValue, newIndex), {before,after}Replace(newValue, index, oldValue), {before,after}Delete(oldValue, index).
  • 4 hooks, {before,after}Update(newValue, index), {before,after}Delete(oldValue, index). (update = add or replace)
  • 4 hooks, {before,after}Add(newValue, newIndex), {before,after}Delete(oldValue, index). This replacements as a delete then add.
  • Variations of the above where all the after hooks are consolidated into one no-argument hook, on the assumption that specs and code will just iterate the new list contents.

I'm not sure what would be best. The adoptedStyleSheets case needs the beforeAdd hook only, for validation. (It doesn't need any after hooks, at least in the spec, since it can just say that the DocumentOrShadowRoot's adopted stylesheets are its adoptedStyleSheetAttribute's backing observable array contents. But implementations would probably benefit from all of them.)


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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>
Copy link
Contributor

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.

@tabatkins
Copy link
Contributor

Strong agree that we need at least a 4-hook model. 2 hooks are unsound; there's nothing theoretically preventing an ObservableArray from allowing undefined in its type signature, and so an undefined can come in via an explicit set. Plus, as shown by your examples in the spec, hooks will generally just immediately branch on whether the value is undefined or not; it's clearer both to read and write if these were separated from the start.

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.

@othermaciej
Copy link

Is it necessary to make these types constructible? This doesn't seem necessary for the additionalStyleSheets use case or anything similar, assuming we are willing to get rid of assignability. With a properly mutable real array, there isn't much need to assign a new array, and doing so is a hazard. (It might be there are other use cases where constructing your own from script is desirable.)

Some thoughts about hooks. Do the after hooks provide anything useful that can't be done with before? Perhaps an alternate model is to think of before as an index setter that can throw or otherwise refuse. Then it can choose to do anything it wants either before or after actually updating the backing store. Maybe for JS-created ObservableArrays this makes sense, since we can't necessarily trust JS to satisfy the Array contract, thus needing hooks around the real setting; but in the case of native implementations, the implementation may want to simply replace the real setting. This would be more natural. Of course, if it was done that way, the implementation would want to replace getting as well. Not sure why this more complex way. Does it help simulate some aspects of array realness that can't be simulated otherwise? Is it solely for the benefit of JS (where a JS callback can't otherwise be trusted to fulfill the Array contract)? A way to make things work in JS without separate before and after callbacks would be to have singular callbacks which get passed a function that performs the operation on an underlying backing store; JS can choose to call it or not.

@domenic
Copy link
Member Author

domenic commented Jan 28, 2020

Is it necessary to make these types constructible?

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 ObservableArray on the platform, but it was unpolyfillable, because authors were not able to construct their own ObservableArrays. We see this problem today with DOMTokenList.

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.

Some thoughts about hooks. Do the after hooks provide anything useful that can't be done with before?

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.

Of course, if it was done that way, the implementation would want to replace getting as well. Not sure why this more complex way.

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.

@othermaciej
Copy link

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.

@tabatkins
Copy link
Contributor

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,

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.

@domenic
Copy link
Member Author

domenic commented Jan 28, 2020

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.

@othermaciej
Copy link

othermaciej commented Jan 28, 2020

@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 length).

@domenic
Copy link
Member Author

domenic commented Jan 28, 2020

(which I think is the only material difference here; all the Array functions would work via index access and length).

The big material difference here is that, unlike indexed getters and setters, here web developers can mutate the array contents. As I said above,

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.

@othermaciej
Copy link

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.

@domenic
Copy link
Member Author

domenic commented Jan 28, 2020

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

The user agent must include all style sheets in the DocumentOrShadowRoot's adopted stylesheets inside its document or shadow root CSS style sheets.

with

The user agent must include all style sheets in the DocumentOrShadowRoot's adoptedStyleSheet's attribute's backing observable array contents inside its document or shadow root CSS style sheets.

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.)

@domenic
Copy link
Member Author

domenic commented Jan 31, 2020

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. building.employees.pop() is defined (by ECMAScript) to be equivalent to

// 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:

  • Contain holes, forcing specification authors to deal with them; or
  • Be a more complicated structure, e.g. a list of (item, indexInTheArray) tuples or a map of indexInTheArray -> item entries, so that the Web IDL spec machinery can properly keep it synchronized. And this more complicated structure would be a nightmare for specification authors to deal with, as e.g. you couldn't append to it without figuring out the proper index, or remove the first item without shifting down all the others.

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.

@bzbarsky
Copy link
Collaborator

bzbarsky commented Feb 1, 2020

Interesting. I would have expected pop() to set the length before deleting...

In any case, I think it's possible to implement pop() without allowing holes: all that's required is that the [[Delete]] of the object change the length before it returns (redundantly with the Set that's coming up) if the last item is being deleted and throw if any other item is being deleted otherwise.

I think that would also allow shift to work, based on a quick skim through the algorithm. And I think splice would work as well, since it does its deletes back to front...

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.

@othermaciej
Copy link

pop() can't change the length first, because that would violate the Array invariant that length is always numerically greater than any indexed property, as defined here.

(Ok, technically for Arrays it could just set length, since that has the side effect of deleting equal or greater index properties. But since it's written to be generic, it tries to simulate the invariant even when operating on other objects that may not magically update length.)

I have no suggestions for the broader problem of how to deal with holes.

@domenic domenic changed the title Introduce the observable array type Introduce the observable array type (legacy platform object based) Feb 3, 2020
@domenic
Copy link
Member Author

domenic commented Mar 13, 2020

Let me close this, as #840 is much better in my opinion, and is better-reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants