Incomplete iterable DOM changes for discussion

[loganfsmyth]

Posting for discussion given babel/babel#4616. Not a complete fix because I wanted to get your thoughts before going further.

As far as I can tell, and given the behavior in Chrome and FF, these are supposed to be iterable because of the behavior documented in https://heycam.github.io/webidl/#es-iterators for IDL. It says any integer-indexed class should be considered iterable automatically, but only those explicitly marked Iterable in the IDL should get keys, values and entries.

Given that, the current behavior of of MediaList, StyleSheetList, and CSSRuleList aren't quite right since they should not have an .keys, .values and .entries. Only NodeList and DOMTokenList are explicitly Iterable<> in the DOM spec.

I've pulled in a list of DOM classes that are currently iterable in Chrome/FF and merged them together with a bit of cleanup. This covers HTMLCollection and NamedNodeMap, which are probably the main important ones people are likely to want to use.

[zloirock]

Thanks! Sorry, I missed your comment in this discussion. If it should be added by Web IDL - we should add it. I'll check docs when I'll have time. Removing methods is a breaking change, but anyway, I want to work on a major release.

[loganfsmyth]

Yeah, wasn't sure what your thoughts were on that .keys etc change. Would you be open to landing a change like this if we just leave the existing ones as true for now for compatibility with existing code? I'm hesitant to wait on a 3.x release since we'd have to decide whether we could adopt it for Babel 6 anyway.

[zloirock]

Would you be open to landing a change like this if we just leave the existing ones as true for now for compatibility with existing code?

Yep.

I'm hesitant to wait on a 3.x release since we'd have to decide whether we could adopt it for Babel 6 anyway.

Most likely it would be problematic - too many breaking changes in proposals and I wanted to add some breaking changes to API. So most likely it's for babel@7.

[loganfsmyth]

@zloirock Could you give me some pointers on the filesystem structure? I'm confused about why there are three files

• modules/web.dom.iterable
• modules/library/web.dom.iterable
• library/modules/web.dom.iterable

The last two add NAME to Iterators but don't mutate the proto since it is a global. What effect does that do in this case? Do I need to expand those lists with the larger set of names?

[zloirock]

modules/web.dom.iterable

It's for global polyfill.

modules/library/web.dom.iterable

It's for the version without global namespace pollution if it's deferred from the global version.

library/modules/web.dom.iterable

It's final library version generated as copying modules and, after, modules/library to one folder at the build time. Ugly, but it works :-)

So you should change the first 2 and add tests if it's not hard.

[loganfsmyth]

Ah okay, library/modules/web.dom.iterable was the main one that confused me, didn't realize it got copied.

For the logic in modules/library/web.dom.iterable, how does it affect iterability of the globals? Is there internal logic somewhere else?

[zloirock]

how does it affect iterability of the globals?

It uses Object.prototype.toString for getting an internal class name and uses matched iterators.

[zloirock]

...and looks like that in one case I forgot to add one additional check for toString value, so we have some minor and hidden, but pollution in old engines. I should review my old code one more time :-)

[loganfsmyth]

@zloirock I assume I'm doing something dumb, mind taking a look and telling me why these tests are failing now?

[loganfsmyth]

@zloirock Thoughts?

[zloirock]

@loganfsmyth sorry, I had some problems with time and I missed your message. I'll check it in the evening.

[loganfsmyth]

No rush, I figured it had gotten lost.

[zloirock]

In library version, we don't need keys / values / entries methods, so we can replace it with an array and get rid of _object-keys dependency.

[loganfsmyth]

Yeah I was on the fence, I figured keeping the two implementations similar was better but totally your call.

[zloirock]

I assume I'm doing something dumb, mind taking a look and telling me why these tests are failing now?

Looks like the problem that npm test does not contain copy task. I missed it because I usually run build task before commits. Need to add it.