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.define() should return the new custom element #927

Closed
rpivo opened this issue May 22, 2021 · 7 comments
Closed

CustomElementRegistry.define() should return the new custom element #927

rpivo opened this issue May 22, 2021 · 7 comments

Comments

@rpivo
Copy link

rpivo commented May 22, 2021

If calling customElements.get() returns undefined, and if we want to immediately set the custom element afterward, we would have to do something like:

    let registeredElement = customElements.get(element);
    if (!registeredElement) {
      customElementRegistry.define("text-element", Text, { extends: "p" });
      registeredElement = customElements.get(element);
    }

If CustomElementRegistry.define() returned the defined element, we could instead do:

    let registeredElement = customElements.get(element);
    if (!registeredElement) {
      registeredElement = customElementRegistry.define("text-element", Text, {
        extends: "p",
      });
    }

Or:

const registeredElement =
  customElements.get(element) ??
  customElementRegistry.define("text-element", Text, { extends: "p" });
@dominiccooney
Copy link
Contributor

Very nice idea, how did we miss this?

@domenic What's the best place for this issue... Here? whatwg/html?

@markcellus
Copy link

markcellus commented May 23, 2021

Hmm this confuses me. define() doesnt create any element, just registers the ability to create custom elements from registry. So there wouldn't be an element to return. Or do you mean it should return the constructor?

@rniwa
Copy link
Collaborator

rniwa commented May 23, 2021

Hmm this confuses me. define() doesnt create any element, just registers the ability to create custom elements from registry. So there wouldn't be an element to return. Or do you mean it should return the constructor?

I think the proposal is to return the second argument. I'm a bit hesitant to adding something like that because it closes the possibility of future extensions to define. e.g. We have proposals / discussions for lazily defining custom elements: #782. We may decide that the way to do that is to let define accept a Promise as the second argument.

@dominiccooney
Copy link
Contributor

Hmm this confuses me. define() doesnt create any element, just registers the ability to create custom elements from registry. ... do you mean it should return the constructor?

Yes. The example code makes it clear the proposal is to return the constructor.

We have proposals / discussions for lazily defining custom elements: #782.

Don't those proposals suggest a new entrypoint for lazy definitions? But to be conservative, sure, maybe resolve #782 first and then do this.

@domenic
Copy link
Collaborator

domenic commented May 23, 2021

In general on the web platform this "chaining" style, where modifier functions return one of their arguments, is discouraged. If you have the argument already, having two ways to get it---the value you passed in, and the return value---just makes for unreadable code.

In particular, the OP seems based on the misconception that customElements.get() returns something special. It does not. In the example given it just returns Text, which the code already has access to.

@rpivo
Copy link
Author

rpivo commented May 26, 2021

@domenic you are right -- I misunderstood that Text would be the same returned value as the argument passed in.

In any case, it would be nice if there was some kind of getOrSet API here. If a custom element is already defined, and if I don't want an error to be thrown, I have to wrap a customElements.define() call in a try/catch.

I can also call customElements.get() first to see if that gets anything, and if it doesn't, then call define() (edit: this patterns seems pretty common - GitHub's open source web components do this: https://github.com/github/clipboard-copy-element/blob/main/src/index.ts#L14-L17).

This is similar to the TC39 emplace proposal, which also lists some similar APIs in other languages: https://github.com/tc39/proposal-upsert

@rpivo
Copy link
Author

rpivo commented Jul 3, 2021

Coming back to this -- I've changed my mind and think this should be closed since the error thrown from customElements.define() is valuable information. The pattern I suggested here hides that information.

@rpivo rpivo closed this as completed Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants