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

Inconsistent enumerability on keys/values/entries and friends #738

Closed
domenic opened this issue Jun 24, 2019 · 10 comments · Fixed by #1166
Closed

Inconsistent enumerability on keys/values/entries and friends #738

domenic opened this issue Jun 24, 2019 · 10 comments · Fixed by #1166

Comments

@domenic
Copy link
Member

domenic commented Jun 24, 2019

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

@bzbarsky
Copy link
Collaborator

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.

@domenic
Copy link
Member Author

domenic commented May 5, 2022

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

@domenic
Copy link
Member Author

domenic commented Jul 13, 2022

@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 web-platform-tests/wpt@31f5f5e 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 added a commit that referenced this issue Jul 13, 2022
This makes the properties generated by the maplike/setlike declarations consistent with properties generated by other IDL constructs such as operation and attribute declarations. (That includes, e.g., explicitly-specified has/get/set/delete operations!) This is also consistent with the generated properties for iterable declarations.

The @@iterator symbol property remains non-enumerable, like it is for iterable declarations, and like all other symbol-named properties are.

Closes #738.
@yuki3
Copy link

yuki3 commented Jul 13, 2022

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

@saschanaz
Copy link
Member

saschanaz commented Jul 13, 2022

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

saschanaz commented Jul 13, 2022

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?

@domenic
Copy link
Member Author

domenic commented Jul 13, 2022

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

@saschanaz
Copy link
Member

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

@domenic
Copy link
Member Author

domenic commented Sep 28, 2022

OK, I am going to count that as Gecko support :). In that case #1166 is ready for review, and I will work on a test-update PR as well.

domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 28, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 28, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 28, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 28, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
@petervanderbeken
Copy link

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.

domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 28, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
domenic added a commit that referenced this issue Oct 3, 2022
This makes the properties generated by the maplike/setlike declarations consistent with properties generated by other IDL constructs such as operation and attribute declarations. (That includes, e.g., explicitly-specified has/get/set/delete operations!) This is also consistent with the generated properties for iterable declarations.

The @@iterator symbol property remains non-enumerable, like it is for iterable declarations, and like all other symbol-named properties are.

Closes #738.
domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 11, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
foolip pushed a commit to web-platform-tests/wpt that referenced this issue Oct 11, 2022
In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 21, 2022
…hecks, a=testonly

Automatic update from web-platform-tests
Expand iterable<> and async iterable<> checks (#36124)

In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
--

wpt-commits: 4e5209fdff87f351bfcbfe765ed669a6e9eeb996
wpt-pr: 36124
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 26, 2022
…hecks, a=testonly

Automatic update from web-platform-tests
Expand iterable<> and async iterable<> checks (#36124)

In particular, check the enumerable/configurable/writable-ness of all the methods.

Related to whatwg/webidl#738.
--

wpt-commits: 4e5209fdff87f351bfcbfe765ed669a6e9eeb996
wpt-pr: 36124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants