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

CustomElementRegistry.prototype.define<bulk>() #892

Open
leobalter opened this issue Aug 12, 2020 · 9 comments
Open

CustomElementRegistry.prototype.define<bulk>() #892

leobalter opened this issue Aug 12, 2020 · 9 comments

Comments

@leobalter
Copy link

Refs:

In the light of discussing Scoped Custom Element Registries, some folks, including @caridy, @justinfagnani, and I, believe it might be interesting to design a method for multiple elements definition.

This would allow us to define a batch of components within a single method call.

customElements.define<bulk>({ // naming TBD
  'x-foo': Foo,
  'x-bar': Bar,
});

// equivalent to

customElements.define('x-foo', Foo);
customElements.define('x-bar', Bar);

Existing Feedback

From @justinfagnani:

This would be a welcome feature even for the global registry. We've had users hit problems when trying to define multiple elements that interact (ie. children firing events to register with parents). They end up being sensitive to the registration order. There's also a performance concern with multiple tree walks to register multiple elements. Bulk registration would help with both.

Compatibility

Unfortunately, we cannot simply modify the existing CustomElementRegistry#define() without breaking changes. Taking this, we might need a new method, using a new name, without replacing .define(), which remains designed for single definitions.

Today, define casts the first argument to string and this also applies to object values:

customElements.define({
    toString() {
      return 'my-element';
    }
  },
  MyElement
);

// similar to

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

Naming Bikeshed

Naming is hard, and this idea is not an exception.

For now I can think of .makeDefinitions, .defineBulk, .defineMultiple, .defineMany, but I'd love better suggestions out of not being a big fan of any of the current ideas.

Alternative

We could just modify .define to not cast the first argument to string if the method is called with only a single parameter.

@rniwa
Copy link
Collaborator

rniwa commented Aug 12, 2020

Why isn't calling customElements.define(~) multiple times not sufficient? What kind of ordering issues does this pose?

@justinfagnani
Copy link
Contributor

The problems I've seen have to do with elements that coordinate and differences in upgrade order depending on definition order or whether the elements are in the main page, or created via innerHTML. With innerHTML upgrades happen in document order, and in the main page upgrades happen in definition order.

Bulk registration would allow upgrades to happen in document order for that set of elements even in the main page.

@rniwa
Copy link
Collaborator

rniwa commented Aug 13, 2020

The problems I've seen have to do with elements that coordinate and differences in upgrade order depending on definition order or whether the elements are in the main page, or created via innerHTML. With innerHTML upgrades happen in document order, and in the main page upgrades happen in definition order.

So to me, we need to discuss this issue before jumping onto any solution. From what you're saying, even if we were to add some capability to bulk define custom elements, we still need to coalesce the upgrades, and that's really the key feature here.

Perhaps what we need is an option to not upgrade all the connected elements in the document upon the call to customElements.define. The author can then call customElements.upgrade(document) once all custom elements have been defined.

@justinfagnani
Copy link
Contributor

Agreed we should talk the underlying problem.

Deferring upgrades would be welcome too, especially as a whole-app optimization. We have customers where up-front element definition, plus upgrade for each definition, has a noticeable perf impact.

I do think that bulk-registration is nice for its locality though: a single library can bulk register a few cooperating elements without turning off upgrades globally.

@rniwa
Copy link
Collaborator

rniwa commented Aug 13, 2020

I do think that bulk-registration is nice for its locality though: a single library can bulk register a few cooperating elements without turning off upgrades globally.

Adding an option to customElements.define would achieve exactly that. It would not turn off upgrade in the future calls to customElements.define unless the same option is specified.

@trusktr
Copy link
Contributor

trusktr commented Aug 31, 2020

I've spent countless hours battling code order issues (while making these elements), often using techniques like microtask-deferral, which further complicates things. Anyone making substantial CE libs is highly likely to encounter the issues. A solution to this would be a huge gift.

@rniwa
Copy link
Collaborator

rniwa commented Aug 31, 2020

I would be really useful to enumerate the list of individual issues you've countered rather than broad "code order issues".

@leobalter
Copy link
Author

I can't immediately break down code order issues here, I can think of a different optimization this method might achieve, where a bulk definition prevent dirtiness, such as sanitization happens before an actual definition.

Let's get this initial example:

customElements.define('x-foo', Foo);
customElements.define('x-bar', Bar);

If we assume an exception happens for the second line, e.g. x-bar was already defined before. I mind end up with x-foo defined but a different x-bar existing. Today, to overcome this problem I'd need to verify if all the names are valid and not defined before I define all I need for my application. In this example I'd need to verify if x-foo and x-bar are defined before I start calling .define for both of them. All of this considering sync execution.

The bulk definition resolves this problem with a single unified step to verify. This becomes more simple as the bulk definition method is finally reliable.

try {
  customElements.defineBulk({
    'x-foo': Foo,
    'x-bar': Bar,
  });
} catch {
  // no new definitions expected here
};

if one of the items is invalid or the name already exists, this method won't pollute my customElements registry.

@rniwa
Copy link
Collaborator

rniwa commented Sep 1, 2020

I can't immediately break down code order issues here, I can think of a different optimization this method might achieve, where a bulk definition prevent dirtiness, such as sanitization happens before an actual definition.

Let's get this initial example:

customElements.define('x-foo', Foo);
customElements.define('x-bar', Bar);

If we assume an exception happens for the second line, e.g. x-bar was already defined before. I mind end up with x-foo defined but a different x-bar existing. Today, to overcome this problem I'd need to verify if all the names are valid and not defined before I define all I need for my application. In this example I'd need to verify if x-foo and x-bar are defined before I start calling .define for both of them. All of this considering sync execution.

The bulk definition resolves this problem with a single unified step to verify. This becomes more simple as the bulk definition method is finally reliable.

Okay, but worrying about cases in which your custom element's local name had already been used by someone else doesn't seem like a common scenario to me. Also, this could be trivially implemented like this:

for (elementClass of elementsToRegister) {
    if (customElements.get(elementClass.name))
        throw Error;
}
for (elementClass of elementsToRegister)
    customElements.define(elementClass.name, elementClass);

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

5 participants