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

Serialization of "collections" #1347

Open
jugglinmike opened this issue Nov 5, 2018 · 6 comments
Open

Serialization of "collections" #1347

jugglinmike opened this issue Nov 5, 2018 · 6 comments

Comments

@jugglinmike
Copy link
Contributor

Serialization of "collections"

In this patch for WPT, we discovered an incongruity between the spec and the implementations. @ato dutifully followed up with a patch to resolve this, but that may be incomplete. A new issue seems like a more intuitive place to discuss than a merged pull request, so here it is!

@ato wrote:

The effect we want to achieve with this PR is that those things we define as
“collection types” should be expanded to an array of items, for which we
invoke the serialisation algorithm again on each item.

In Marionette we implement this in JS using [...collection] because it
works on all of the collection types, including HTMLAllCollection despite it
not carrying an iterable declaration. I would love some feedback on how you
define “expand all these things to arrays, as if calling [...collection] in
JS”.

HTML uses a narrower definition of what a collection is because it limits
them to things deriving from NodeList if I remember correctly. In particular
this excludes the Arguments object, but more importantly, there is not a
centralised list of collections in HTML or DOM, and this is an attempt an
enumerating those, plus adding Arguments into the mix.

“type” probably should be replace with “an instance of”.

That explains why the latest test references the "Iterable" interface. However, the text doesn't actually use that interface; it reads a length property and accesses additional string properties based on that. That's why prior to your explanation, I was planning to recommend we remove the requirement.

If we're targeting the behavior of [...collection], then we'll need to explicitly specify the use of the iterator protocol. That will ensure that all implementations do the right thing when (for instance) iterator.next throws an error. ECMAScript demonstrates how this is done in the runtime semantics for SpreadElement.

This is much more complicated (conceptually) than simple property access. If we're going to commit to it, I wonder if it's worthwhile to accept any iterable. This will be easier to specify, but more importantly, it will be an easier model for developers to internalize. @ato @gsnedders what do you think?

@jugglinmike
Copy link
Contributor Author

There are a few different heuristics for identifying "collections" that have precedence in ECMAScript:

The current text (and gh-1351) is a little different than all of these.

The collection-identifying text combines aspects of the first (through the attempt to identify Array instances) and the third (through the requirement on the iterable interface) and adds a few special cases.

The collection-serializing text references the "length" and integer properties, making it more like Array.prototype.slice.

Would it be possible to use one of these heuristics instead? As mentioned above, that would be easier for developers to internalize since they already know what "array like" and "iterable" mean. While they could name a new concept like this (e.g. "WebDriver collection"), I don't know if the needs of this specification warrant the cognitive overhead that entails. (It would also relieve editors of the unenviable task of enumerating the things WebDriver can enumerate).

IsArray is clearly too strict to use directly, but I think we could choose from either of the latter two. It seems that WebIDL effectively makes all "array-like" DOM objects iterable, so I think either would be compatible with existing implementations.

@gsnedders
Copy link
Member

Either of the latter two would be a significant change, though.

In the former case:

{
  length: 1,
  0: "a",
  foo: "bar"
}

This will now return ["a"].

In the latter case, you have that problem but even more extreme (e.g., consider things like Map which provide iterators, where we almost certainly don't want that behaviour).

@jugglinmike
Copy link
Contributor Author

I can see how the first example might conflict with existing code, but it seems less likely that people are currently returning Map instances and expecting empty objects. Why do you say that we almost certainly don't want to enumerate entries?

@gsnedders
Copy link
Member

@jugglinmike oh, wait, what happens with Maps currently?

@jugglinmike
Copy link
Contributor Author

Chrome and Firefox both serialize Maps (and Sets) as empty objects, regardless of their contents.

@jugglinmike
Copy link
Contributor Author

That seems to be in-line with the current specification text.

AutomatedTester pushed a commit that referenced this issue Nov 22, 2018
Fixes one of the problems identified in
#1347.
AutomatedTester pushed a commit that referenced this issue Nov 22, 2018
Similarly to #1351, we want
to run instance checks on JS Array and Object.

This patch relates to #1347.
andreastt added a commit that referenced this issue Nov 22, 2018
Arguments is technically not a prototype, and the test is for the
toString result.

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

No branches or pull requests

2 participants