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

HTML element constructors need to Get(newTarget, "Prototype") #560

Closed
rniwa opened this issue Aug 31, 2016 · 7 comments
Closed

HTML element constructors need to Get(newTarget, "Prototype") #560

rniwa opened this issue Aug 31, 2016 · 7 comments

Comments

@rniwa
Copy link
Collaborator

rniwa commented Aug 31, 2016

The current spec says HTMLElement constructor needs to use the prototype stored by customElements.define.

This is not the way regular constructor works in a super() call. Furthermore, this semantics is really cumbersome to implement in WebKit / JSC due to the way our engine's abstractions are designed.

@rniwa
Copy link
Collaborator Author

rniwa commented Aug 31, 2016

There was a consensus to store callbacks because other implementors didn't want to detect when the presence of callbacks change over time to avoid enqueuing custom element reactions when not necessary as necessary optimizations but this would not be a thing for the prototype internal slot since we always need to get it when constructing an element.

@WebReflection
Copy link

not sure if relevant but when I've written about the super (and transpilers) problem, since we discussed this on twitter (where you seem to be disappeared) I've noticed that in Canary it's possible to create custom elements without even extending.

// no extend
class MyElement {}

// definition
customElements.define('my-element', MyElement);

// here we go
const me = Reflect.construct(
  HTMLElement,
  [],
  MyElement
);

Surprisingly this doesn't throw. I'm not sure it's a Reflect gotcha or there's really something wrong with the way inheritance works with HTMLElement constructor.

Apologies if off topic.

@rniwa
Copy link
Collaborator Author

rniwa commented Aug 31, 2016

That's not supposed to throw. That's one of those Reflect.construct quirks. But please go file another issue instead of continue discussing it here since that's off topic as you mentioned.

@domenic
Copy link
Collaborator

domenic commented Aug 31, 2016

So the first thing to figure out is when these two behaviors could possibly be distinguishable. They can't if you use class syntax, because in that case .prototype is non-configurable. So I guess we're talking about the specific case of function declarations that use Reflect.construct in their body and whose .prototype is then changed after element definition.

The other thing to consider is consistency with other constructors, as you point out. An example makes it clear that as you say, it's the current value of the .prototype property that is used when constructing new instances, not the original.

Another way of looking at this is, if we were to think about "self-hosting" built-in elements with custom elements, or even just making built-in elements constructible, what would their behavior be? I'd like to think that overwriting HTMLParagraphElement.prototype would mean that new HTMLParagraphElement or <p> elements would be created with the new prototype for everyone; this is an intrinsic part of the contract of the new operator and object creation in general. On the other hand, overriding HTMLImageElement.prototype.connectedCallback would not change the browsers behavior for fetching images when you connect the image to the DOM. This is all hypothetical, as HTMLParagraphElement.prototype is non-configurable and we haven't worked to add callbacks for all the connected/attribute changed/etc. behavior to any classes in HTML. But as a hypothetical, this makes sense to me.

All this together indicates to me that @rniwa's instincts are right and we should change the spec to use new.target.prototype at construction time, instead of looking up the stored prototype. And furthermore, to me this is not inconsistent with the desire to store the callbacks.

@dominiccooney @kojiishi, would you be able to change Blink's implementation to do the new.target.prototype lookup instead of using the registry? I have the start of a test case for the new proposed spec for you here, which Chrome Canary fails: https://jsbin.com/subogesoji/edit?html,console,output

Now, let's work on the details here. In particular, what should happen if .prototype has been set to null or a non-object? I think the correct thing to do is fall back to the prototype of the HTML constructor that is being extended, as that's consistent with what ES does (as shown in this example which falls back to Error.prototype). We could also consider falling back to the prototype stored in the definition...

@dominiccooney
Copy link
Contributor

I haven't tried to implement this yet, but this basically sounds fine. We'd like to know what to do if the prototype property is "invalid" in some way. (Is prototype ever configurable? Could it throw, too?)

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 1, 2016

It's not configurable but writable in regular function but it could do anything because you can create a Proxy that traps Get to prototype. e.g.

customElements.define('bad-element', new Proxy(class extends HTMLElement {}, {
  get: function (target, name) {
    if (name == 'prototype')
      throw 'stuff';
  }
}));

@domenic
Copy link
Collaborator

domenic commented Sep 1, 2016

Yeah, so if it throws, we should rethrow. (That also matches ES.) But what if it returns a non-object? To match ES it should fall back to the [HTMLConstructor] currently executing, I think. Does that sound reasonable to implement?

Following https://tc39.github.io/ecma262/#sec-getprototypefromconstructor exactly, the logic is:

  1. Let prototype be Get(NewTarget, "prototype"). Rethrow any exceptions.
  2. If Type(prototype) is not Object,
    1. Let realm be GetFunctionRealm(NewTarget).
    2. Set prototype to the [interface prototype object] of realm whose [interface] is the same as that of the [active function object].

(Using [] to denote links to spec terms in IDL/ES.)

Note that in the last step realm can be different from [the current Realm record], which is even more subtle fun. I'll be sure to write some test cases.

domenic added a commit to whatwg/html that referenced this issue Sep 6, 2016
This changes the prototype used when constructing a custom element instance from being the one saved at element definition time, to the one derived from NewTarget (with fallback semantics similar to JavaScript when new.target.prototype is not an object). Fixes WICG/webcomponents#560.
domenic added a commit to whatwg/html that referenced this issue Sep 6, 2016
This changes the prototype used when constructing a custom element instance from being the one saved at element definition time, to the one derived from NewTarget (with fallback semantics similar to JavaScript when new.target.prototype is not an object). Fixes WICG/webcomponents#560.
domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 6, 2016
rniwa pushed a commit to web-platform-tests/wpt that referenced this issue Sep 6, 2016
domenic added a commit to whatwg/html that referenced this issue Sep 15, 2016
This changes the prototype used when constructing a custom element
instance from being the one saved at element definition time, to the one
derived from NewTarget (with fallback semantics similar to JavaScript
when new.target.prototype is not an object). Fixes
WICG/webcomponents#560.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This changes the prototype used when constructing a custom element
instance from being the one saved at element definition time, to the one
derived from NewTarget (with fallback semantics similar to JavaScript
when new.target.prototype is not an object). Fixes
WICG/webcomponents#560.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants