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

[scoped-registries] reduce pain by preventing logic from ever touching non-upgraded custom elements #960

Open
trusktr opened this issue Jul 3, 2022 · 6 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Jul 3, 2022

Needing to handle pre-upgrade properties and values for custom elements whose instances are used before they are upgraded requires hideous difficult-to-read and error-prone code.

Pre-upgrade values are the bane of custom elements, if not a nightmare. They are the single most difficult aspect of writing custom elements and I keep seeing people encounter issues with all the time.

Even after years of writing custom elements, I still encounter issues myself.


I think it would be much better if we did something like the following ideas:

  • when calling scopedRegistry.define('some-element', SomeClass), an error is thrown and the call fails if any elements with the name some-element already exist in the ShadowRoot
  • using cloneNode() on a template that contains custom elements should throw an error when an element would be created without being upgraded
    • Perhaps cloneNode() can accept a new arg that allows specifying which registry to using during the clone process, f.e. cloneNode({ deep: false, registry: scopedRegistry })
  • document.createElement would have a new registry option, although the existing without such option can't be changed. Framework authors can opt into doing document.createElement(..., {registry: someRegistry}) (and similar with cloneNode) in which case errors can be thrown on undefined elements.
  • An alternative to shadowRoot.innerHTML should throw if the parsing encounters an unknown custom element
  • We should provide new APIs that encourage good defaults. For example, ShadowRoot.createElement() should throw on unknown elements, and we can add APIs like ShadowRoot.cloneElement(elementToClone) (also element.scope.cloneElement(element)) to clone elements with the error-throwing defaults (as alternative to cloneNode), etc
  • To simplify things, we can add a new option to attachShadow, f.e. el.attachShadow({ allowUnknown: false }) that will cause all of a the ShadowRoot's APIs (even existing ones like innerHTML that we otherwise can't break) to throw errors when they'd otherwise create unknown elements.
  • I'm not sure what to do with declarative shadow roots, but my hunch is that their processing would remain the same, including instantiating unknown elements, but an error would be thrown the moment a developer tries to access them (using any old APIs with new options, old APIs under the new attachShadow allowUnknown:false option, or new ShadowRoot APIs).
    • For example, parsing could create the ShadowRoots including their sub trees, but something like shadowRoot.querySelector() or shadowRoot.firstElementChild would throw if they were to return an unknown element.
    • If the user doesn't touch any APIs that would return a non-upgraded element, no error is thrown, therefore rendering still works, and any pre-existing Document or ShadowRoot APIs would continue to be backwards compatible.
  • etc

By "unknown element" I mean any element that has a hyphen in its named that would otherwise be a custom element if the registry it participates in had the defined class for that name.

In other words, apart from top level customElements and existing document and ShadowRoot APIs which we can't break, it should otherwise be impossible to get an instance of a custom element that is not upgraded; errors should always be thrown in any possible case that creates unknown element instances; and all current APIs should gain an option that allows them to throw any time they would create an unknown element.


This would greatly, I mean vastly, improve the developer experience for custom element authors, and even for authors of major well-known frameworks that, to this day (Jul 3, 2022), I see dealing with this stuff. In fact, authors of major frameworks

I can't tell you how much time I've spent dealing with pre-upgrade issues! The cost of that lost time is at least in the thousands, but more importantly it's an annoying and frustrating thing I don't want to spend any time on at all.


@justinfagnani The explainer does not mention any changes that are similar to what I described above; there is a potential here to greatly reduce developer code complexity.


Related:

#922

#923

@trusktr trusktr changed the title Please, PLEASE, do not allow custom elements to be upgraded in scoped ShadowRoot registries. [scoped-registries] sparing scoped registry users from the pain of pre-upgrade properties Jul 3, 2022
@trusktr trusktr changed the title [scoped-registries] sparing scoped registry users from the pain of pre-upgrade properties [scoped-registries] *reduce the pain:* preventing logic from ever touching non-upgraded custom elements Jul 3, 2022
@trusktr trusktr changed the title [scoped-registries] *reduce the pain:* preventing logic from ever touching non-upgraded custom elements [scoped-registries] reduce pain by preventing logic from ever touching non-upgraded custom elements Jul 3, 2022
@justinfagnani
Copy link
Contributor

This seems to have nothing to do with scoped registries does it?

Or do you want to add new behavior to scoped registries because it would be a breaking change on the global registry? Seems like new behavior could be triggered by a new option to ElementDefinitionOptions.

@rniwa
Copy link
Collaborator

rniwa commented Jul 19, 2022

I guess we could consider adding a new mode of custom element registry that doesn't support upgrading?

@justinfagnani
Copy link
Contributor

Would that be better there or with in ElementDefinitionOptions?

It's probably more likely that each element definition has a better idea of whether it can handle things like properties set before update. All Lit elements for example come with code to handle that. A registry creator might not have that information about the elements in it.

So customElements.define('my-tag', MyTag, {upgrade: false}) might be better.

@rniwa
Copy link
Collaborator

rniwa commented Jul 20, 2022

So that would mean we'd have a mixture of elements that get upgraded & not upgraded in a single tree. That sounds very confusing for users of those components to me. For the behavior to make sense, it really needs to be done per registry, not element by element. Otherwise, each custom element will have to worry about not just which custom element is defined but also how they're defined before using them. That seems to make things more complicated, not less.

@trusktr
Copy link
Contributor Author

trusktr commented Jul 29, 2022

This seems to have nothing to do with scoped registries does it?

Yes it does, because scoped registries presents an opportunity to make better APIs, whereas the existing document APIs can not be modified without breaking the web.

Or do you want to add new behavior to scoped registries because it would be a breaking change on the global registry? Seems like new behavior could be triggered by a new option to ElementDefinitionOptions.

Oh yes, you answered that. 😄 Writing as I read.

Yes indeed, we could add new options, but for the documet registry it would have to default to the value we don't want. For scoped registries, it could default to that value we do want to avoid the upgrade issues by default.

@trusktr
Copy link
Contributor Author

trusktr commented Jul 29, 2022

or the behavior to make sense, it really needs to be done per registry, not element by element

I think this makes sense, because it will dictate exactly how the author of a ShadowRoot will load elements. In the case of forbidding upgrade, then the author will simply have to ensure that all definitions are ready before setting a root's innerHTML, or similar.

Typically this will already be the case for authors that import element definitions, because definitions will be available before the author's custom elements are even defined.

But there may be cases where an author loads a script synchronously or similar, and in those cases they should wait for scripts to be loaded first.


Another question is: do we want the engine to throw a helpful error to console at the point that a definition becomes available and there are elements that won't be upgraded? Error: Skipping element upgrade. Ensure you load custom element definitions before creating any of those elements. or similar.

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

3 participants