The custom element state when constructor thrown

[kojiishi]

The create an element for a token step 7 says when create an element thrown:

let element be instead a new element that implements HTMLUnknownElement, with no attributes, namespace set to given namespace, namespace prefix set to null, custom element state "undefined", and node document set to document.

Then when this element is inserted, the insert a node tries to upgrade this element.

Shouldn't we prevent to upgrade elements whose constructor thrown?

@domenic @dominiccooney

[domenic]

Interesting. So the concrete test case is

<script>
let shouldThrow = true;
customElements.define("x-bad", class XBad extends HTMLElement {
  constructor() {
    super();
    if (shouldThrow) {
      throw new Error("bad");
      shouldThrow = false;
    }
  }
});
</script>

<x-bad></x-bad>

Per the current spec this will try to create an XBad, then fail, but then upon insertion, try to "upgrade" from HTMLUnknownElement to XBad, and succeed. This is especially weird since the "upgrade" goes laterally across the hierarchy (i.e. from HTMLUnknownElement to XBad, instead of from HTMLElement to XBad).

One "solution" would be to say that this is an OK edge case that emerges from the semantics, but we should probably clean up the potential for a lateral transition, so make it HTMLElement instead of HTMLUnknownElement.

Otherwise, if we want to say that throwing in your constructor means we should never try to upgrade you, then the most straightforward solution I can think of is to add a new custom element state, "unupgradeable", which we set if the constructor fails. Then any future upgrades or try-to-upgrades would bail out if they see this state. We would probably also use this in other cases, such that if upgrading fails once, we should never try it again.

What do people think would be better?

[trusktr]

We would probably also use this in other cases, such that if upgrading fails once, we should never try it again.

Probably should still try again, as the error may have been due to something missing or not in place at runtime, but could be in a future point in time, in which case upgrading would work (new constructor may not throw at that point).

[rniwa]

I don't think we should try to update an element in the case it had failed to construct.

[domenic]

@rniwa so just to make sure I understand, you are in favor of adding an "unupgradeable" state, to ensure that any constructors that fail are never invoked again?

[kojiishi]

I agree with @rniwa that we shouldn't run constructor mulitple times on the same element.

One more thing we need to take care of, if we were to add a new state, is that whether the new state is defined or not. Semantically, it is not defined. However, the expected most common use case of defined is not(:defined) to find elements yet to be upgraded, as in the example in element definition. From this purpose, I would like it to be defined.

I mildly prefer it be defined for the reason above, but I'm ok either way.

And if we were to make it as defined, we could use uncustomized too. I'm ok either way on this too.

[domenic]

Hmm. I think it would be better to be not defined. After all, it will not work like a defined element would. It has not yet been upgraded (and never will be). :not(:defined) would be better off matching it, I think. For example if people are trying to hide elements until they are interactive.

[domenic]

PRs up to fix this at whatwg/dom#282 and whatwg/html#1558. It turned out to be fairly straightforward and fit with existing mechanisms nicely.

[trusktr]

It seems to me like preventing the constructor from ever running again is not the best thing to do. What if some error state elsewhere in the app causes the constructor to fail, and then that error state goes away, in which case the element should come alive again the next time it is used.

Or, does this explicitly prevent the constructor from running twice on a specific element such that if I removed <some-element> then attached a new <some-element> then the new one will have it's constructor called?

class SomeElement extends HTMLElement {
  constructor() {
    if (!someStateInTheApp) {
      throw SomeError('...')
    }

    // ...
  }
}

Would the best practice for a library that writes a constructor like that be to encourage the failed element to be removed and for the user to try again by adding a new element?

[andyearnshaw]

Or, does this explicitly prevent the constructor from running twice on a specific element such that if I removed then attached a new then the new one will have it's constructor called?

Yes.

Would the best practice for a library that writes a constructor like that be to encourage the failed element to be removed and for the user to try again by adding a new element?

I think the best practice would be not to rely on specific states the app may or may not be in while your constructor is running. Keep your constructor as light as possible (only setting up its own state and tree, for instance) to avoid potential errors. Add initialisation and cleanup routines to callbacks.