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] Concerns about non-construction of scoped elements #987

Closed
chalbert opened this issue Mar 9, 2023 · 14 comments
Closed

Comments

@chalbert
Copy link

chalbert commented Mar 9, 2023

My primary concern is how risky moving a library from the global registry to a scoped registry would be.

My secondary concern is that it discourages libraries from using tag-abstract reference to other custom elements.

Examples

  1. Library registered on global registry
    This example works correctly, as expected.

  2. Library registered on scoped registry
    The same component moved to a scoped registry throws an run-time error, hidden inside a hard to test flow.

Details

Moving a library from the global registry to a scoped registry would be dangerous unless you have total confidence that it has been tested on a scoped registry. Unit tests and test on a global registry would not be sufficient. Like in the example 2, bugs could happen deeply and not be easily identifiable. Upgrading a library would be as dangerous. Of course libraries that always use shadowDOM with a custom registry would be safe from this, but there may still be to use cases where it is not possible, and anyway as it is allowed, we can assume it will happen.

For my secondary concern, what is the recommended way for a element to reference a component of the same library (outside shadowDOM) without assuming the tagName of this component?

@xiaochengh
Copy link

I think the issue is largely due to library having no knowledge about how its components are registered, and therefore, has no way to create them.

This can be solved if we can look up the registered local name from a registry, namely, with CustomElementRegistry.getName() proposed in #566.

Your example can be changed into

class ComponentB extends HTMLElement {
  static observedAttributes = ['foo'];
  attributeChangedCallback(name, oldValue, newValue) {
    if (name === 'foo' && newValue === '') {
      console.log('ComponentB: "foo" attribute added');
      try {
        let registry = this.getRootNode().registry || window.customElements;
        let tagName = registry.getName(ComponentA);
        this.appendChild(this.getRootNode().createElement(tagName));        
      } catch (error) {
        console.error(error.message);
      }
    }
  }
}

@chalbert
Copy link
Author

Thanks for you answer! It does answer part of my secondary concern and provide a nice way for creating a component without assuming its localName.

But if I'm not mistaken, it would only work once the component is connected, correct?

@xiaochengh
Copy link

But if I'm not mistaken, it would only work once the component is connected, correct?

Correct. So ComponentB also has to do something in connectedCallback().

@justinfagnani
Copy link
Contributor

Components generally shouldn't be creating their own light-dom children. Their children are in their parent's root node's scope, and "belong" to that scope, not the component.

But if there is a special case where a component creates it's own light-dom children, it can just create the element by tag name:

const el = this.shadowRoot.createElement('component-a');
this.append(el);

The only thing the absence of a constructor prevents is calling new ComponentA(), it doesn't prevent creating the element at all.

@bathos
Copy link

bathos commented Apr 14, 2023

shadowRoot.createElement

@justinfagnani is this method being exposed by ShadowRoot part of the proposal?

@justinfagnani
Copy link
Contributor

Yes, please see https://github.com/WICG/webcomponents/blob/gh-pages/proposals/Scoped-Custom-Element-Registries.md#scoped-element-creation-apis

@chalbert
Copy link
Author

The only thing the absence of a constructor prevents is calling new ComponentA(), it doesn't prevent creating the element at all.

I'm aware of that. My point is:

  • Libraries can create components from constructor on global registry. A component working on the global registry won't necessarily work on a scoped registry, or could stop working in the future, unless it has been specifically designed not to use constructors.

@xiaochengh
Copy link

@chalbert Am I understanding it correctly that both this issue and #989 are trying propose the idea that each constructor should be restricted to one registry, and:

Anyway, I do not support this idea. The major reason is that it can cause conflicts in diamond dependency cases, while the whole purpose for scoped registries is to avoid registration conflicts. Treated differently, if a constructor can only be registered at one place, it's in some sense still global, which means it will cause conflicts and is something scoped registries want to avoid.

Without this restriction, libraries directly creating light-dom children can still be migrated -- though not a straightforward task.

Libraries can create components from constructor on global registry. A component working on the global registry won't necessarily work on a scoped registry, or could stop working in the future, unless it has been specifically designed not to use constructors.

I see the point. With scoped registries, direct usage of constructors would probably become an anti-pattern that we want to strongly discourage -- sort of a deprecation. Then it might need similar care like usage data etc.

@chalbert
Copy link
Author

@xiaochengh You are correct that both issues are related, but I was trying to address independent aspect separately. Hopefully I haven't created too much confusion.

But my issue wasn't proposing a specific solution, but was really meant to raise a concern, and see if hopefully there were some alternatives. One major challenge of using WC in large applications is to import the correct components, and only the component actually used. This is much easier to enforce with direct references through imports than when depending on existing definitions.

Restricting constructor to one registry is not an ideal solution, l agree. Are there any alternatives?

@justinfagnani
Copy link
Contributor

I really think we need to clarify the potential problem first before searching for a solution.

The proposal is designed as is in part to prevent breaking existing pages and components.

So:

  • Globally registered constructors will create elements in the global scope, as they do today
  • Constructors not registered globally will throw, as they do today. This is especially important because a constructor could be registered globally after attempted calls, and that behavior must match.
  • Registering a constructor in a scoped registry will not prevent it from being registered in the global registry, so using scoped registries will not change the behavior of code using the global registry
  • Inheritance of registries through the DOM hierarchy was removed form the proposal to ensure that components using the global registry aren't effected by an ancestor using scoped registries.

The only thing the absence of a constructor prevents is calling new ComponentA(), it doesn't prevent creating the element at all.

I'm aware of that. My point is:

Libraries can create components from constructor on global registry. A component working on the global registry won't necessarily work on a scoped registry, or could stop working in the future, unless it has been specifically designed not to use constructors.

If a library creates components with the global registry today, it will continue to work with no behavior changes, regardless of what any scoped registry does.

But this is where I'd need the risk clarified with some concrete examples. What does it mean for a library to "work on a scoped registry"?

I see two possibilities:

Are you talking about a library that calls constructors it imports? In that case it will work the same in the presences of scoped registries. Constructors you import from a module will (most likely) be either globally registered or not registered at all. I think it would be pretty unusual to export a pre-scoped constructor, though it's possible to create today (see next example).

Or, are you talking about a library that accepts arbitrary constructors and called new on them? Something like:

const makeElement = (ctor) => new ctor();

While possible, I haven't really seen this in the wild, however for this to work with scoped registries, then the caller will have to do something like:

const registry = new CustomElementRegistry();
registry.define('my-element', MyElement);
class MyScopedElement extends MyElement {
  constructor() {
    return registry.createElement('my-element');
  }
}

makeElement(MyScopedElement);

(I believe this will work, can you confirm in your prototype @xiaochengh?)

Yes, this is awkward, because we intentionally left open the question of how to handle scoped constructor. One of the original options was to have define() return a scoped constructor to make this simpler:

const registry = new CustomElementRegistry();
const MyScopedElement = registry.define('my-element', MyElement);

makeElement(MyScopedElement);

I think this is still an interesting option, but we didn't want to block the proposal on this given that we can add it after.

@xiaochengh
Copy link

xiaochengh commented Apr 17, 2023

This is how I understand the situation:

  1. A library defines two components CompA and CompB, where one may create another as a light-dom child via constructor
  2. Another developer creates another component, which uses CompA and CompB in its shadow tree. The shadow root also has a scoped registry with CompA and CompB registered.

Then the components break. The developer has no way to fix it on their side, but can either stop using scoped registries or wait for the lib to migrate.

This means the current design of scoped registries is not fully backward-compatible.

@justinfagnani Your examples are still making constructor calls use a specific registry, and do not solve the case.

@xiaochengh
Copy link

We discussed this in the last F2F, and after that @justinfagnani and I also discussed it offline.

When the issue does occur, it indeed leads to difficulties in migration, although still possible (see previous comments in this thread).

However, we think this doesn't lead to any major concern, because the issue relies on a pattern (component creating light-dom children) that is very rare (according to @justinfagnani's experience), and it's not a good pattern even without scoped registries as it breaks encapsulation. Also, the pattern (relying on a global constructor) seems fundamentally incompatible with any scoping efforts.

So we'll leave the current design of scoped registries as is.

The worst outcome is that such libraries will remain unmigrated, and they will still work as is without scoped registries. As this is a rare pattern, it shouldn't affect the adoption of scoped registries.

@chalbert
Copy link
Author

chalbert commented Apr 19, 2023

Thanks for your answer and the time you've given to this concern.

As a parting note on this issue, here's some work I'm doing that uses JSX syntax to build WC from constructors.

export default class AppLayout extends HTMLElement {
  // Implements a rendering protocol that allows any compatible renderer to be used, e.g. jsx or lit.html
  @render()
  render({ children }) {
    const navs = [
      { url: '/url1', text: 'Link A' },
      { url: '/url2', text: 'Link B' },
    ];
    return (
      <>
        <AppHeader>header</AppHeader>
        {/* custom attributes, experimental jsx syntax */}
        <nav {CompositeWidget}={{ foo: 'bar '}} {AttachElement}="foo; bar">
          {navs.map((nav) => (
            <div>
              <a href={nav.url}>{nav.text}</a>
            </div>
          ))}
        </nav>
        {children}
        <AppFooter>footer</AppFooter>
      </>
    )
  }
}

Instead of limiting constructors to a single registry, I've moved to using a special undetermined element that get's upgraded once the node is connected. It means it also becomes possible to reuse templates statically between registries:

export default class AppEntry extends HTMLElement {
  @render()
  static template = (
    <AppLayout>
      <HomePage />
    </AppLayout>
  )
}

The render decorator also looks for undeterminedConstructors attached to the template result and automatically defines undetermined elements on the elements's scoped registry.

I've got a prototype that seems to work correctly, although it's still very early. Let me know if you are interested in getting updated on my progress.

@keithamus
Copy link
Collaborator

keithamus commented Apr 21, 2023

As has already been discussed, WCCG had their spring F2F in which this was discussed. I'd like to point out that you can read the full notes of the discussion (#978 (comment)) in which this was discussed, heading entitled "Scoped Custom Element Registries".

@chalbert chalbert closed this as not planned Won't fix, can't repro, duplicate, stale Apr 21, 2023
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