webidl icon indicating copy to clipboard operation
webidl copied to clipboard

Inconsistent enumerability on keys/values/entries and friends

Open domenic opened this issue 5 years ago • 8 comments

Keys/values/entries in particular:

I'm not sure whether consistency with other methods on the interface prototype object should win (enumerable) or consistency with ES (non-enumerable). I guess I'd lean toward the former?

This gets worse actually for maplike and setlike, where all the "generated" methods seem to be non-enumerable. This includes ones with innocuous names like has, get, the size getter, ...

I wonder if browsers actually follow the spec here :-/.

domenic avatar Jun 24 '19 01:06 domenic

I wonder if browsers actually follow the spec here :-/.

It's a bit exciting to test given lack of use for maplike/setlike in the wild... Gecko implements nothing that's setlike, for example, and I think the only default-exposed maplike is RTCStatsReport. For FontFaceSet Gecko fakes setlike via regular interface members, which are of course enumerable.

Within that constraint, this testcase:

<pre>
<script>
    let objs = [URLSearchParams, RTCStatsReport, FontFaceSet];
    let props = ["keys", "values", "entries"];
    for (let obj of objs) {
      for (let prop of props) {
        document.writeln(obj.name, ".", prop, ": ",
                         Object.getOwnPropertyDescriptor(obj.prototype, prop).enumerable);
      }
    }
</script>

shows that Gecko does in fact make the iterable case enumerable and the maplike case non-enumerable like the spec says. In Safari the behavior is like Gecko (including having FontFaceSet stuff enumerable, but maybe it doesn't use setlike there either?).

Chrome has FontFaceSet as [NoInterfaceObject]. If I use document.fonts.constructor instead of FontFaceSet, the testcase shows that Chrome makes all these methods enumerable, looks like, so it's not following the current spec.

I don't have strong opinions about the desired behavior here.

bzbarsky avatar Jun 25 '19 00:06 bzbarsky

I've added tests for the currently-specced behavior (Firefox passes, Chrome fails, unsure yet on Safari) to https://github.com/web-platform-tests/wpt/pull/33951.

domenic avatar May 05 '22 22:05 domenic

@EdgarChen @saschanaz @yuki3 @shvaikalesh let's settle this once and for all. This is trivial but is an interop issue and consistency issue and now I started testing it in https://github.com/web-platform-tests/wpt/commit/31f5f5e952f84df73a9673c5ecc2e835d4ba44dc and that caused a bunch of WPT idlharness failures :).

Here is my proposal:

  • maplike:
    • size, entries, keys, values, forEach, get, has, set, delete, clear: enumerable (*)
    • @@iterator: non-enumerable
  • setlike:
    • size, entries, keys, values, forEach, has, add, delete, clear: enumerable (*)
    • @@iterator: non-enumerable
  • iterables:
    • entries, keys, values, forEach: enumerable
    • @@iterator: non-enumerable

(*) = change from current Web IDL spec.

Does this sound good? Explicit thumbs-up votes appreciated, especially from implementers.

domenic avatar Jul 13 '22 01:07 domenic

IIUC, the proposal is what Blink currently implements. +1.

yuki3 avatar Jul 13 '22 04:07 yuki3

~~I think Blink is actually failing the new test per https://wpt.fyi/results/webrtc/idlharness.https.window.html?diff&filter=ADC&run_id=5769845135114240&run_id=5708320617791488. Gecko passes that so I have no objection.~~ (@EdgarChen is currently the better person to look at this, though.)

saschanaz avatar Jul 13 '22 11:07 saschanaz

Oops, so that's just testing the current specced behavior and this wants to change that to match Blink. In that case I want our IDL people to take a look. (@EdgarChen and @petervanderbeken)

Is there some discussion log about why maplike/setlike things are non-enumerable?

saschanaz avatar Jul 13 '22 11:07 saschanaz

I think we copied what the ES spec does, which is generally to make everything non-enumerable (including normal methods).

domenic avatar Jul 13 '22 11:07 domenic

Since nobody responded... If we are okay with such deviation (I definitely see some people hate that, #1179 😅), then I'm okay with this.

saschanaz avatar Sep 18 '22 01:09 saschanaz

OK, I am going to count that as Gecko support :). In that case https://github.com/whatwg/webidl/pull/1166 is ready for review, and I will work on a test-update PR as well.

domenic avatar Sep 28 '22 07:09 domenic

Sorry, missed this thread. I don't think I have a strong opinion here. The difference with the ES spec is unfortunate, but WebIDL is already different for operations anyway.

petervanderbeken avatar Sep 28 '22 07:09 petervanderbeken