[scoped-registries] Interaction with HTML element's overridden constructor steps

[xiaochengh]

This is a blocking issue of #716

Note: This issue is not about calling a custom element constructor directly

---

HTML element's overridden constructor steps is called whenever we create a custom element. For example:

class MyCustomElement extends HTMLElement {
  constructor() {
    super(); // Runs the overridden constructor steps
    this.attachShadow(...);
    ...
  }
}
customElements.define('my-custom', MyCustomElement);

The line super() is reached whenever we create a my-custom element, regardless of calling createElement(), setting innerHTML or calling the constructor directly.

The overridden constructor steps check the (unique) global CustomElementRegistry to find out if there's a custom element definition matching the currently called constructor, and throw an exception if there's none.

This gets trickier when there are multiple registries. For example, when we call ShadowRoot.createElement() and enter the super() call, we still need to know which registry we are using, but currently such information seems unavailable.

One very hacky idea:

• Whenever we are about to create a custom element with a given registry and definition, temporarily tag the constructor with the registry as an internal field [[Registry]]
• Then when we are inside super(), we can get the correct registry from NewTarget's [[Registry]] field
• When the overridden constructor steps finish, clear the [[Registry]] field from NewTarget

@mfreed7 @justinfagnani @rniwa

[justinfagnani]

This sounds very similar to the constructor-call trick used to upgrade custom elements. I assume it'll have the same limitations, in that constructing another custom element of the same class before the super() call would be dangerous.

[kevinpschaaf]

+1, in the polyfill I used a global stack to communicate the registry to the super(), but storing a slot temporarily on the NewTarget seems better.

Like Justin said, the only thing would be to make sure the browser treats calling new on the same NewTarget before super() as an error (since that would construct what should use the global tagName as the scoped tagName). Justin and I discussed today, and that might just be as simple as checking that the tagName of the returned instance is the same as the one expected from the registry set before calling new, and erroring otherwise.

[xiaochengh]

I'm implementing the idea of adding a temporary slot on NewTarget in the Chromium prototype.

Also, when the constructor re-enters (even including @justinfagnani's case), if we make it work like an AutoReset, namely, keep the current value before re-entering the constructor (and thus rewriting the slot), and restore the previous value when the re-entered constructor finishes, then we won't be calling super() with a wrong definition.

I'm adding a new WPT in my patch to test these cases, and it seems to be working well.

[xiaochengh]

Please let me know what you think about this approach.

If it looks good, I'll try to land that patch and also revise the spec draft accordingly. Thank you!

[xiaochengh]

Unfortunately I found a case where my approach doesn't work...

It uses a constructor that re-enters itself with a direct call, and it's registered to different registries with different names:

let nestedCalled = false;
let createdByNestedCall;
class ReentryByDirectCall extends HTMLElement {
  constructor() {
    super();
    if (!nestedCalled) {
      nestedCalled = true;
      createdByNestedCall = new ReentryByDirectCall;
    }
  }
}
window.customElements.define('test-element', ReentryByDirectCall);

let registry = new CustomElementRegistry;
registry.define('shadow-test-element', ReentryByDirectCall);
let shadow = host.attachShadow({mode: 'open', registry});

Then if we call shadow.createElement('shadow-test-element'), we are supposed to also create createdByNestedCall using the global registry, and hence set its local name to test-element.

However, with the approach, we just check the tag on the constructor, and ended up using the scoped registry and creating another shadow-test-element

---

I feel like it's impossible for the UA to tell who made the constructor call in this case, and the only way out is to disallow any re-entry of the same CE constructor.

Is there any real use case with CE constructor re-entry? If I understand correctly, re-entry is just an artifact that we happen to allow, but has no real use at all, because it either creates infinite recursion, or needs to rely on something external (external variables or even the DOM) as the recursion boundary, which looks like a really bad design.

[chalbert]

Here's an idea that seems to work, although I'm not sure it can be implemented.

If scoped registries were to define a sub-class of the passed component, adding to it a reference to the registry, it would allow the HTMLElement constructor to identify the registry on which the component is registered.

let preSuperCalled = false;
let preSuperComponent;
let postSuperCalled = false;
let postSuperComponent;

const $registry = Symbol('[[Registry]]');

class CustomElementRegistry {
  #registry = {}

  constructor(name) {
    this.registryName = name;
  }

  define(name, Component) {
    if (customElements === this) {
      this.#registry[name] = Component;
    } else {
      this.#registry[name] = class extends Component {
        static [$registry] = registry;
      }
    }
  }

  get(name) {
    return this.#registry[name];
  }
}

const customElements = new CustomElementRegistry('global');

// HTMLElement has a default registry field
class HTMLElement {
  static [$registry] = customElements;

  constructor(name) {
    console.log(`Component "${name}" on registry "${this.constructor[$registry].registryName}"`);
  }
}

class ReentryByDirectCall extends HTMLElement {
  constructor(name) {
    if (!preSuperCalled) {
      preSuperCalled = true;
      preSuperComponent = new ReentryByDirectCall('pre-component');
    }
    super(name);

    if (!postSuperCalled) {
      postSuperCalled = true;
      postSuperComponent = new ReentryByDirectCall('post-component');
    }
  }
}

customElements.define('test-element', ReentryByDirectCall);

let registry = new CustomElementRegistry('custom');

registry.define('shadow-test-element', ReentryByDirectCall);

new (registry.get('shadow-test-element'))('shadow-custom-component');

Working codepen.

This works for components created from constructor both pre and post super() call.

---

The downside would be that if the constructor is accessed from within a custom-scoped component, it will not be the original constructor but the subclassed one. Or maybe that could become a feature as it could be useful to know if a custom element is used on the global registry or a custom one.

Looking forward to using scoped registries!

[bathos]

The downside would be that if the constructor is accessed from within a custom-scoped component, it will not be the original constructor but the subclassed one.

Note that in the demo the subclass also has a unique prototype inheriting from the original and the constructor has its own new name and length properties. While this pattern doesn’t require the registry-specific prototype, it’s awkward to omit it because if that were done, new instance.constructor would use the global registry. More importantly though, so would any use of ReentryByDirectCall inside the class body — that binding identifier would be pointing at the original constructor regardless.

If a solution like this is used, I think it would be safer and clearer for authors if it were realized by disallowing the same constructor-by-identity from being in multiple registries: subclasses (or equivalent) would be required, as above, but it would fall on the author to decide how they’re achieved. By requiring that, it ends up apparent in the ES code where the “identity breaks” will occur and they can be accounted for if needed. The “automatic” approach seems pretty surprising & mysterious to me relative to that.

[chalbert]

I agree with you, preventing the same constructor from being defined on multiple registries is clearer. This seems a reasonable limitation, I personally prefer that to stoping constructor re-entry, as edge case at it is. There already seems to be an association between the constructor and its registry as only a defined custom element can be instantiated through its constructor, this seems like a logical extension. I could see the use for some helpers though: CustomElementRegistry.getRegistryOf, CustomElementRegistry#isRegistryOf, etc.

[xiaochengh]

I think it would be safer and clearer for authors if it were realized by disallowing the same constructor-by-identity from being in multiple registries

Thanks for the idea. That solves the issue nicely!

To help me understand it better, what is the downside if we don't allow constructor re-entry though? (other than changing the existing behavior)

[chalbert]

I may be a question of preference, but I find that definition-time error would be caught earlier and thus be clearer than component run-time. And as you've pointed out, it would would also avoid breaking with the current standard that allows calling a constructor from itself.

But most of all, I think it would be a shame that elements registered on custom registries wouldn't be able to be created using constructors at all.

I personally often use this kind of dependency injection pattern:

class AbstractConfirmBar extends HTMLElement {
  constructor({  SubmitButton, CancelButton }) {
    super():
    this.SubmitButton = SubmitButton;
    this.CancelButton = CancelButton;
  }
  connectedCallback() {
     this.appendChild(new this.SubmitButton());
     this.appendChild(new this.CancelButton());
  }
}

Such a component would work as expect with global-registered components, but not with custom-registered components. There are other ways to achieve the same result, but in the end the real problem is the inconsistency. Some patterns would simply be forbidden when a component becomes registered on a custom registry, even if it worked perfectly fine until then.

[justinfagnani]

I don't think prohibiting constructors from being used in multiple registries is a good idea. It means that we'll need trivial subclasses for each registry (and there could be 100s of registries, resulting in 1000s of new subclasses), and that it would be strictly dangerous to expose a custom element class because anyone who registers it without subclassing would cause an error for other consumers.

We talked this part through in several meetings in the context of how new MyElement() should work, and considered a variant where CustomElements.define() automatically subclasses and returns the subclass, but rejected that in favor of new MyElement() only working for the global registry and coming back to how to make scoped constructors work later.

Thus the line in the example that's causing a problem should not actually be a problem, I think:

  createdByNestedCall = new ReentryByDirectCall;

because this can only create an instance in the global registry, and because this call is after super(), the tag applied to the constructor should have been cleared (it should be cleared before the return from the HTMLElement constructor).

Constructor before super() could be ambiguous, but those are already dangerous due to the constructor call trick used during upgrades. I think it's fine to have the same restriction for scoped registries.

[chalbert]

Thanks for the clarification.

If I understand correctly, this means that any component coming from code you don't control (e.g. 3rd party library) will need to be adjusted to avoid using constructors? That a component that currently works on the global registry may throw an error if moved to a custom registry? That the error may happen deeply in the component during a rare and hard to test flow, thus causing bugs that could even reach production? This does sound dangerous.

I can see that reusing the same constructor could cause some exceptions, but there would all be definition-time and likely to be caught early. Knowing that custom registries are not yet available, it is unlikely that they will spread so far and wide that suddenly libraries you don't control cross-register the same constructor. The worst case scenario I can see is that a developer tries to redefine a constructor, see that it throws, that then subclasses it.

For the number of sub-classing required, I wonder if we don't overestimate the problem. I may be mistaken, but the use case for scoped components wouldn't be primarily for privately defined sub-components? I would expect libraries to discourage the reuse of those components outside the library itself. In this scenario, no subclassing would even be required.

The most annoying scenario I see would be if lib A and B both use components from lib C. To avoid collisions, both A and B would need a mechanism to use components from C without exposing them to globally.

Could registries extend other registries? In the previous example, instead of having both A and B having to redefine every components from C, it would be much simpler to define which registries you extend.

const myRegistry = new CustomElementRegistry({ extends: [registryC, registryD] });

Where element lookup happens in order, from the current registry up the extended registries, in order. It would be easy for libraries to expose a registry with every components already registered.

Was something like that considered? Maybe you could help me understand other scenarios for re-registering the same constructor multiple times?

[justinfagnani]

If I understand correctly, this means that any component coming from code you don't control (e.g. 3rd party library) will need to be adjusted to avoid using constructors?

No. It just means that you can't recursively call your own constructor before super() - which is already a restriction due to how upgrades work.

[bathos]

No. It just means that you can't recursively call your own constructor before super() - which is already a restriction due to how upgrades work.

While it wouldn’t be advisable or useful to do so, it’s not a runtime restriction (and seemingly can’t be, at least in terms of ES code semantics) because there’s nothing to key a specific expected invocation of HTMLElement.[[Construct]](args, newTarget) to an associated registry except for the identity of its newTarget argument.

The example below shows how the intended registries can get swapped as a consequence — pretend new Foo("scoped") is being invoked with a scoped registry:

let elements = new Map;
let callCount = 0;

class Foo extends HTMLElement {
  constructor(key) {
    if (!callCount++) new new.target("“global”");

    elements.set(`${ key } call, ${ !elements.size ? "scoped" : "global" } registry`, super());
  }

  static {
    customElements.define("x-x", this);
  }
}

new Foo("“scoped”");

elements; // Map(2) {'“global” call, scoped registry' => x-x, '“scoped” call, global registry' => x-x}

This is similar to what @xiaochengh described above. If this was already apparent, are you saying it’s okay if it’s not guaranteed that the registry associated with the instance super() furnishes will match the registry associated with the outer CE constructor invocation? If so, would it be possible to make it throw if a registry-associated invocation ends up returning an element associated with a different registry?

[justinfagnani]

Recursively calling a constructor before super() can get individual instances swapped already (new new call will be attached to the in-document, currently upgrading instance instead of a new disconnected instance). Since it's already a terrible idea to do, I think it's ok to not continue to not support this for scoped registries.

pretend new Foo("scoped") is being invoked with a scoped registry

This isn't a thing in the current proposal. Calling constructors is only supported for the global registry: https://github.com/WICG/webcomponents/blob/gh-pages/proposals/Scoped-Custom-Element-Registries.md#custom-element-constructors

[bathos]

This isn't a thing in the current proposal. Calling constructors is only supported for the global registry:

Yes, I meant pretend it’s the internal invocation of Foo (hence the labeling argument). It’s not the most artful demo, admittedly :)

Recursively calling a constructor before super() can get individual instances swapped already.

Right, in the upgrade path, but a TypeError is thrown if the expected instance isn’t returned (“...must not return a different object”). That’s why I asked about whether a similar exception could/would be thrown in this scenario.

[chalbert]

No. It just means that you can't recursively call your own constructor before super() - which is already a restriction due to how upgrades work.

This isn't a thing in the current proposal. Calling constructors is only supported for the global registry

It comes back to what I was saying, unless you are sure your component will be exposed globally, using constructors is not safe anymore.

// This lib exposes 2 public components, but does not know if there are going to be defined 
// globally or scoped, nor the tag used. The only expectation is that they are going to be 
// defined somewhere accessible.
class A extends HTMLElement {}
class B extends HTMLElement {
 connectedCallback() {
     this.appendChild(new A());
  }
}
export { A, B };

Component A works globally or scoped, B will work globally but fails when scoped.

Wouldn't it be a good practice for libraries not to assume their component is globally defined, nor the actual tag it is defined under?

[justinfagnani]

This isn't a thing in the current proposal. Calling constructors is only supported for the global registry

It comes back to what I was saying, unless you are sure your component will be exposed globally, using constructors is not safe anymore.

Maybe we're having a mis-communication here. I'm not saying that it's unsafe: it's specifically disallowed.

When you call new MyElement() an instance will be created in the global scope if MyElement is registered there, otherwise it will throw. This is true today, so constructors are no more or less unsafe than they are now.

The example you have is incomplete because it doesn't show registrations. Today for this to work, you'll need:

class A extends HTMLElement {}
customElements.define('x-a', A);

class B extends HTMLElement {
 connectedCallback() {
     this.appendChild(new A());
  }
}
customElements.define('x-b', B);
export { A, B };

This will work today and when scoped registries are added. Scoped registries cannot break this currently working code.

If you want it to use scoped registries, you still need registrations, and again, because calling constructors never uses a scoped registry, you need to use one of the scoped element creation APIs:

class A extends HTMLElement {}
const registry = new CustomElementsRegistry();
registry.define('x-a', A);

class B extends HTMLElement {
  constructor() {
    this.attachShadow({mode: 'open', registry});
  }
  connectedCallback() {
    this.appendChild(this.shadowRoot.createElement('x-a'));
  }
}
// global registration for <x-b> is optional
customElements.define('x-b', B);
export { A, B };

Wouldn't it be a good practice for libraries not to assume their component is globally defined, nor the actual tag it is defined under?

Yes, this is the practice that scoped registries are trying to enable, and the solution is for a parent element to choose the tag name that it registers dependencies under and use that tag name to create instances.

Again, we did consider an API where CustomElements.define() would return a scoped constructor. Based on feedback from the group, we decided to not add that in the first version and instead have constructors only work for the global scope. If we want to revisit that decision, I think that is a slightly different discussion than re-entrancy.

[xiaochengh]

No. It just means that you can't recursively call your own constructor before super() - which is already a restriction due to how upgrades work.

Hmm, didn't notice there's such a restriction. Then the problem can also be solved.

Let me see how to implement this.

[chalbert]

Ok, sorry for the off-topic discussion, I'm leaving it at that.

[xiaochengh]

Now (I think) I finally understand how the construction stack works, so it's a pretty natrual idea to adapt it for this issue:

• Move the construction stack from definition to constructor
• Each stack entry is augmented into either an (element, definition) pair, or an already-constructed marker
• At the beginning of the overridden constructor steps, we get the defintion from the NewTarget by:
  • If there's a non-empty construction stack on NewTarget and the last entry is not the already-constructed marker, use the definition in that entry
  • Otherwise, try to find a definition from the global registry

There's also some detail that I need to verify:

The construction stack actually always throws if there's a recursive constructor call, regardless of whether it's before or after super():

• If it's before super(), later the outer super() will see an already-constructed marker and throw
• If it's after super(), then the inner super() will see an already-constructed marker and throw
  You can see the behavior in test case https://jsfiddle.net/gu81kdw5/

I'm not sure if this matches the intention of the spec, which only says before-super calls are bad. But I guess it doesn't matter, because there's no real use of constructor recursion. If it doesn't match the original intention, then we can just change the intention and recognize the current behavior.

(I've also put up an implementation patch, which will land if everything above looks fine to you)

[xiaochengh]

Closed as resolved.