whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.09k stars 2.66k forks source link

window.customElements.define retrieves the lifetime callbacks in an odd order #3580

Open mrbkap opened 6 years ago

mrbkap commented 6 years ago

window.customElements.define, step 10.3 specifies how to retrieve the lifetime callbacks from the constructor's prototype. The order that is specified seems semi-arbitrary, though (maybe somewhat semantic).

I think that it would be better to follow the example of WebIDL dictionaries instead. WebIDL specifies that dictionary members should be ordered lexicographically.

The easy fix would be to re-order the callbacks in step 10.3 to be in alphabetical order.

I don't think this would be a breaking change, as I find it hard to believe any existing code would

domenic commented 6 years ago

What is the motivation for changing this, and causing all the existing engines to change their code, the spec editor to write new text, and new tests to be written?

smaug---- commented 6 years ago

Making the custom elements follow more closely the behavior in other DOM APIs. Also, it would be good to use webidl there, if possible.

smaug---- commented 6 years ago

(this is case where not-using webidl for defining APIs easily leads to inconsistent behavior with the other APIs)

annevk commented 6 years ago

@smaug---- define() uses IDL, so I think you'll have to be more specific. Do you mean that IDL should offer type-validation of sorts for constructors? Are there other places that would use it?

domenic commented 6 years ago

As you are aware, the semantics we use here are not expressible in Web IDL.

Furthermore, the claim that alphabetical reordering would lead to more consistency is false---very few dictionaries on the platform are alphabetically ordered.

I'll close this as it seems to be asking for change for change's sake. If a compelling reason to update all existing implementations exist, and someone else is willing to volunteer to write the spec changes and tests, then we can consider reopening.

smaug---- commented 6 years ago

currently in Gecko we use webidl to store the callbacks https://searchfox.org/mozilla-central/source/dom/webidl/WebComponents.webidl#22

WebIDL dictionaries are always handled in an expected order, not somewhat random.

mrbkap commented 6 years ago

@domenic To be clear, I didn't file this as "change for change's sake". This will cause us (Mozilla) to have to write a bunch of additional code to handle this special case.

I'm curious about what you mean by "the semantics we use here are not expressible in Web IDL" -- certainly parts of them are.

To be clear, whether or not the properties in a Web IDL dictionary are specified to be in alphabetical order, the properties are always retrieved from the object (when passing into a Web IDL-implemented interface) in alphabetical order. You can test that with this gist -- the dictionary's w property is retrieved first despite being the last property in the dictionary.

More generally, Web IDL dictionaries exist to describe "property bag"-style APIs, and they already describe, in detail, how to retrieve and convert these values. Furthermore, they serve as documentation for parameters and return values for developers. I don't see why we'd need to reinvent this wheel here.

mrbkap commented 6 years ago

Oh, another difference between dictionary conversion rules and Web Components is the handling of null values -- dictionaries ignore them and WC require that we throw. Is there an argument in favor of this distinct handling?

domenic commented 6 years ago

The short answer for while I'm on vacation is that this is not and was not intended to be a property bag API. It accepts a class, and stores "clean" references to some of its methods for calling later, but isn't meant to be similar to dictionaries or other property bag APIs. Classes are extraordinarily different beasts from dictionaries.

If you want to reuse Gecko's dictionary infrastructure to save yourself some code, that's an interesting implementation choice, but as you've noted, the mismatch between the API's semantics and the implementation technology you want to use will cause you problems.

annevk commented 6 years ago

Classes are extraordinarily different beasts from dictionaries.

If the only observable difference is ordering though I think this warrants some further justification, since we could change the retrieval order here, right?

rniwa commented 6 years ago

Yeah, it seems like it's worth normalizing the retrieval order & null treatment to be consistent with dictionary case. I don't think there is much benefit in being different in this case. In general, we should try to reduce the platform quirks & inconsistencies like this as much as possible.

annevk commented 6 years ago

Reopening since there's now two implementers asking for this.

rniwa commented 6 years ago

I think the strongest argument for fixing this issue is really that this is yet another random inconsistency we have in the platform. The more inconsistencies like this we have, the harder it gets for a new comer to make a good use of the Web platform. I personally see this as a major issue the Web platform is facing right now. We've got too many different programming models (e.g. callback vs. promise) and inconsistent behaviors across our APIs (e.g. throw exceptions vs. ignore).

bzbarsky commented 6 years ago

For what it's worth. I think the null issue is not really an issue: the dictionary Gecko uses here for some reason declared the members as nullable, and if they are not nullable we should simply remove those ? from the dictionary members.

The ordering thing is still there. We can hack our implementation to get the current spec ordering if need be, but it would be extra code and cognitive overhead, obviously.

annevk commented 5 years ago

Unfortunately I had forgotten about this while a bunch of new features were added to https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-define.

It seems we get statics from constructor and callbacks from prototype. Should there be a dictionary for each and then we use https://heycam.github.io/webidl/#es-type-mapping for each object with the appropriate dictionary to get the data out? And then we adjust the logic a bit around that? Or is there a reason Firefox is not doing this for constructor?

cc @tkent-google

annevk commented 5 years ago

A more ambitious take would be a new IDL type as discussed at https://github.com/heycam/webidl/issues/701. That would also help worklets.