Closed Bug 1216751 Opened 4 years ago Closed 4 years ago

provide forEach() method on iterable<> webidl interfaces

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: bkelly, Assigned: bzbarsky)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 4 obsolete files)

14.41 KB, patch
qdot
: review+
Details | Diff | Splinter Review
11.40 KB, patch
qdot
: review+
Details | Diff | Splinter Review
25.24 KB, patch
qdot
: review+
Details | Diff | Splinter Review
7.58 KB, patch
qdot
: review+
Details | Diff | Splinter Review
14.64 KB, patch
qdot
: review+
Details | Diff | Splinter Review
This part of the webidl spec suggests iterable<> interfaces should have a forEach() method:

  http://heycam.github.io/webidl/#es-forEach

It seems there might be some issues with the spec, though.

I think we want to get this resolved, though, as most of the real world code I've seen using iterable on fetch Headers tries to use forEach().
Note to self:  this spec needs a bit of work, since k and kValue are IDL values, not ES values, so they need conversion to ES values before the callback function can be invoked....  In implementation terms, we can either do that in bindings by hand or make the generated forEach method take as its first argument a callback type we synthesize which takes the right IDL arguments and let the binding infrastructure do most of the work.
Oh, and the current spec only makes sense for single-type iterables.  And it seems to get the arguments backwards from every single other forEach in the world (which all have the value first, then the key).

I guess I'll try to write up a sane spec here....
Oh, and the third arg to forEach should be the this value; the callback this value should be undefined...  At least if we want to match every iterable in ES6 that has forEach, as well as Chrome's Headers impl.
Though actually, do we want more arraylike semantics for forEach or more setlike?  In particular, does a removal of a key at or before the point where forEach is called cause some keys to be skipped (like array) or not (like map/set)?

Chrome's Headers impl seems more arraylike in this case:

  var h = new Headers;
  h.append("a", "b")
  h.append("c", "d")
  h.append("e", "f")
  h.forEach(function(...args) {
    console.log(args);
    h.delete("a");
  });

logs:

  ["b", "a", Headers]
  ["f", "e", Headers]

I'm going to start a thread on public-script-coord, because this seems like the sort of thing that actually needs figuring out. :(
Added dev-doc-needed.

When this lands, we have to document forEach() for all occurences of iterator<>, currently FormData, Headers and URLSearchParam.

We will need to update: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Write_an_API_reference/Information_contained_in_a_WebIDL_file#Pair_iterator 
(removing the link to this bug and linking to FormData.forEach().
Keywords: dev-doc-needed
Cameron merged his spec changes for iterable<> yesterday:

https://github.com/heycam/webidl/commit/3c61abe1dda3c157ddf278b312540702277ecc00

/me assigning to Boris because he said he plans to fix this bug this week.
Assignee: nobody → bzbarsky
Argh, this fails tests, because the spec is still wrong.  In particular, https://github.com/heycam/webidl/issues/67 still needs to get fixed...  Updated patches coming up.
Attachment #8718138 - Attachment is obsolete: true
Attachment #8718138 - Flags: review?(kyle)
I think https://github.com/heycam/webidl/pull/90 should spec what I've implemented here.
Attachment #8718136 - Attachment is obsolete: true
Attachment #8718136 - Flags: review?(kyle)
Attachment #8718137 - Attachment is obsolete: true
Attachment #8718137 - Flags: review?(kyle)
Attachment #8718140 - Attachment is obsolete: true
Attachment #8718140 - Flags: review?(kyle)
Attachment #8718200 - Flags: review?(kyle) → review+
Attachment #8718208 - Flags: review?(kyle) → review+
Attachment #8718174 - Flags: review?(kyle) → review+
Attachment #8718139 - Flags: review?(kyle) → review+
Attachment #8718209 - Flags: review?(kyle) → review+
Everything looks good (and simpler/smaller than before!). Sorry for the review delay.
Chris, are we going to need this on branches, or is trunk ok?
Flags: needinfo?(cpearce)
bz: At this stage, trunk is OK. We may want it uplifted later, I'll follow up if we need it uplifted.
Flags: needinfo?(cpearce)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.