-
Notifications
You must be signed in to change notification settings - Fork 378
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
Add Scoped CustomElementRegsitry explainer #865
Conversation
Reviewing the issue thread again, it may be good to add some non-goals, like that this isn't a security primitive. Or should that be a well-known non-goal of Shadow DOM in general? |
```js | ||
const registry = new CustomElementRegistry(); | ||
|
||
and associate them with a ShadowRoot: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and associate them with a ShadowRoot: | |
``` | |
and associate them with a ShadowRoot: | |
```js |
In previous drafts registries were inherited through the DOM hierarchy. Is this intentionally out of the current proposal? I think it's a good thing, but just making sure. If an element is in one shadow root and moves to another shadow root, it's element definition is still the one it received from the initial shadow root, correct? That wasn't fully clear to me from the wording. For the list in
|
It's very much intentional and based on discussion and feedback. The goal is to allow a component to safely operate without interference from other scopes. If a component uses a scoped registry, we presume it doesn't want interference from outer scopes. If it does, then it has to opt into inheritance.
No, it will use the registry from the shadow root it's currently in. Moving elements between scopes is very, very rare.
Thanks for those! I'll add them. |
I mean a situation like this: import { ElementA } from './element-a.js';
import { ElementB } from './element-b.js';
const a = document.body.appendChild(document.createElement('div'));
const b = document.body.appendChild(document.createElement('div'));
a.attachShadow({
mode: 'open',
registry: new CustomElementRegistry({
definitions: { 'my-element': ElementA },
}),
});
b.attachShadow({
mode: 'open',
registry: new CustomElementRegistry({
definitions: { 'my-element': ElementB },
}),
});
a.shadowRoot.innerHTML = '<my-element></my-element>';
const myElement = a.shadowRoot.querySelector('my-element');
console.assert(myElement.constructor === elementA);
b.shadowRoot.appendChild(myElement);
console.assert(myElement.constructor === elementA); So even after moving shadow roots, the constructor stays the same. |
Yes, I don't think there's any desire here to try to swap definitions somehow once they've been created. |
|
||
* `createElement()` | ||
* `createElementNS()` | ||
* `importNode()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about adoptNode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adoptNode
doesn't create nodes or run upgrade steps (https://dom.spec.whatwg.org/#concept-node-adopt) so it doesn't need to find a registry.
|
||
#### Note on looking up registries | ||
|
||
One consequence of looking up a registry from the root at element creation time is that different registries could be used over time for some APIs like HTMLElement.prototype.innerHTML, if the context node moves between shadow roots. This should be exceedingly rare though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sentence was a little confusing at first, maybe need some rewording!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to clean this up.
I don't think inheritance is necessary to get basically all of the benefits and might be worth leaving out of first iteration of this. It opens the door for accidental dependence on transitive dependencies (for example, component set authors creating / exporting a registry with their package's components in one big bundle) and adds complexity to the lookup process that makes it harder to understand where your definitions come from, when you should generally know where the elements you depend on are defined. Separate topic: I'd like to recommend that You can still use object literals for sugar by wrapping them in |
Feedback after debating this update with @diervo and @JanMiksovsky today:
|
It would be great if we could merge the current agreed status. Is there something I can help with? |
@LarsDenBakker thanks for the reminder. For now we are tracking the progress in a separate repo (for multiple specific issues) in here: https://github.com/justinfagnani/scoped-custom-elements Should we update this PR with the new content or perhaps just a link to the new repo? I might be wrong, but the current status is:
cc'ing @justinfagnani @caridy here. what's the best approach for this PR now? |
Thanks for the update. Besides the PR it would also be good to update the issue description here #716 to either reflect the new proposal or link to the new repository. I've had a few people get confused about what exactly the current status of the proposal is. |
Sure thing! For that I need to coordinate w/ @justinfagnani and ask him to update the issue description. I don't have write access to change issues in this w3c/webcomponents repo so it's a tricky job for a team. :) |
after @rniwa's feedback, we are moving things back to this repo. In this case, let's update this PR with the latest explainer from https://github.com/justinfagnani/scoped-custom-elements. |
@leobalter @caridy @LarsDenBakker I just updated the PR with the changes that landed in https://github.com/justinfagnani/scoped-custom-elements |
Thanks! cc @rniwa. I hope this proposal is now a bit closer to fair clarification and up to a point we can work through any technical concerns. I'm gonna try to summarize the current topics after I recompose myself from the TC39 meeting this week, please let me know if there is any other outstanding parts to look at. |
The latest proposal still doesn't answer my question at #716 (comment) |
@rniwa I thought we agreed in the last virtual f2f that shadow roots with scoped registries should not inherit from the main registry - once you add a scoped registry you need to add definitions for all elements you use. So the explainer has this line in the Overview
and is also implied in the * Finding a custom element definition* section:
That might not standout enough though, do you have a suggestion for how to make that more clear? I don't have a section on upgrades because it seems like upgrading is untouched by this change, only the "find a registry" steps are and everything past that is the same. Maybe that's not quite right though and for upgrades the tree walk to find elements to upgrade needs to be modified too? |
@rniwa ofc! by no means I tried to get away with this. My goal was to make sure we have the discussions in this repo. We still have that original issue and this current PR, but none are resolved yet. To sum up with @justinfagnani's comment, I wonder if @caridy's comment at #716 (comment) might help with some potential loose ends. I hope we can work through those parts, and I'm sorry if I gave any impression of not being fair with your concerns. I appreciate the process where we move this forward in collaboration and shared efforts. |
I'm talking about the upgrade ordering. It's unclear in what order custom elements get upgraded when multiple shadow references to a give scope. |
Ok, so I think we're looking for clarification and changes to steps 17 and 18 in the Element definition_ steps: https://html.spec.whatwg.org/multipage/custom-elements.html#element-definition It seems like these steps need to be branched depending on whether the registry is the global registry or a scoped registry. If it's the global registry:
If it's a scoped registry:
I think in the explainer it would make sense to have a new section before Finding a custom element definition like Element definition to describe this. @rniwa does this sound about right? @rniwa @mfreed7 do you have guidance on whether registries should keep references to the associated shadow roots? |
@rniwa I'm still unclear on how is this different from 1 registry (the global one) vs 10 registries? can you elaborate more?
This should not be different to what we do today with the global registry. New entries on those registries can cause upgrades on multiple shadow roots, isn't it? Just like adding a new entry into a scoped registry can cause similar upgrades, in the same fashion, including same ordering. |
The current spec has a definitive order for upgrades. With a scoped registry associated with multiple shadow roots, there are at least two plausible orderings for which shadow roots, and thus which elements, to upgrade first: shadow-including document order, or shadow root creation order (order that shadow roots were associated with the registry).
We may want to pick the option where we use shadow root creation order because we wouldn't have to walk the entire shadow-including document, we could just visit each associated shadow root and consider only descendants. That seems like a faster choice, though there could be an inconsistency with the global registry if we include disconnected shadow roots. Keeping a list of shadow roots with a registry would create a cycle between shadow roots and registries. I don't know if that's something generally avoided in the DOM, which is why I'm asking for the guidance. |
I think supporting multiple shadow roots is the only really important requirement, so that one registry can be shared across all instances of a definition. Other than that, I haven't heard of requests for supporting multiple documents. Sometimes printing / screenshot libraries will clone a DOM tree into an iframe, which is something to consider, but I think the ones that handle shadow roots aren't also registering any elements in the iframe - they're just deeply cloning un-upgraded nodes and adding shadow roots as needed. Agreed that the major cost of definitions is the JS time, but for 100s of definitions and 1000s of elements (largest app I know of is ~1k definitions, ~7k nodes) this could be non-trivial, especially if every definition has its own registry. |
I'm on board with the idea of not allowing sharing registries across documents, we can always expand on that in the future if it becomes an issue. I'm not ok with not allowing sharing a registry between multiple shadow roots. Even though we don't plan to do so at salesforce platform ATM, I believe it will feel very natural for anyone working on a group of components, to share the same registry. Preventing that sounds very restrictive. |
The other reason I like the idea that custom registries use the same tree walk as the global registry is that implementation optimizations will benefit both. It's a good question what should happen when you move a shadow tree across documents. And losing the registry without recourse feels somewhat sloppy. Perhaps we need the equivalent of |
I'm not certain that there is a meaningful optimization we could implement for the global one. The idea of visiting shadow trees in the order they were associated with a custom element registry works because
I'm a bit confused here. You don't need |
I was thinking the global one could do shadow trees with their own registry more quickly, but it depends a bit on nested shadow trees and might require more bookkeeping than it's worth. It does seem like you want that to be fast as well though since not all elements will be nested in a shadow tree. If a registry is bound to a document and you move a shadow root associated with a registry to another document, I would assume we want that shadow root to lose its connection to the registry. I suppose we could also make a bunch of things no longer work, but either way you should be able to recover from that I think (by associating the shadow root with an equivalent registry for that document). For elements we offer |
Well, you'd need nested shadow trees to be ordered so you kind of need to either cache the ordering or traverse through them anyway. The only work you're gonna avoid for free will be leaf shadow trees or shadow trees in which there are no more than one nested shadow tree which uses the global registry. But that's easy enough of an optimizations to add regardless of what we do with scoped registries. It would be really shitty if we had to traverse through the entire document to figure out which shadow tree comes first each time upgrading step needs to run in
What do you mean by "lose its connection"? Meaning that things like innerHTML will no longer work? That's sort of in line with what I was suggesting earlier but for this recovery thing to work, we'd need to make it possible to re-associate a shadow root with a new scoped registry after the fact. That seems like a new complication to me. Overall, there is a lot of open questions with regards to upgrade ordering & multiple documents. |
Yes, it's a complication, but it also seems somewhat necessary for a complete design of this feature. I could see not supporting it if we have a viable path to add it in the next version though. Basically, we need to tackle multiple documents in one of two ways:
(Note that the way custom elements / the global registry offer support here is through the |
So there is another interesting piece here. If a scoped registry has any kind of fallback to the global registry, then it would kill the possibility to ever support inheritance via the tree ancestry. This is because both ancestor shadow roots' scoped registry as well as the global may define a custom element of the same name, and if we ever introduced a direct fallback from a nested shadow root to the global, it won't be backwards compatible to then introduce a fallback through shadow tree ancestors. This feature seems to pose a lot of tricky design pitfalls... We may need to think about the whole problem space (as in everything we may want to do in the future) before we can even come up with MVP. Someone should try prototyping this in some browser and figure out how it works out in practice too. This is a very tricky API to design. |
I think this is one of the motivations behind not having a fallback. Once you have a scoped registry you have to fully specify all definitions. Evolving from there any inheritance or fallback would have to be opt-in. |
Certainly that's the case here! And we are counting on that! Honestly, I don't think we will want to explore the fallback mechanism in the future, I hope we don't, but we will see! |
@justinfagnani could you maybe update the PR to include these design points? Both the open questions around ordering and how to deal with multiple documents and why we're not handling fallback (right now). I guess we should have merged this and discussed these things in dedicated issues, but here we are. It seems you also need to associate your account with a W3C account, although given that this repository isn't really bound to a Working Group afaik I'm not sure that matters a whole lot... |
if we get this merged I can work through these additional topics with @caridy in a quick follow up. Otherwise I might direct a patch to @justinfagnani's branch. The goal is not to raise pressure but facilitate the process. Please let me know what would work best. |
I opened #895 to redirect the design issues in the current specs being highlighted here. Maybe this could unblock merging this current explainer as well?
@annevk I'm adding this bit to the explainer this week, if it does get merged before that, I'll have a direct PR here. |
@annevk and @leobalter
I can. I'm a bit swamped right now, so it'll be a few more days. Could we go with merging and opening issues like @leobalter has done with #895? Otherwise he has merge permissions on my fork. Whatever's best. |
You have to resolve the ipr check before I can merge this. To be clear, I don't think these issues are orthogonal in any way. They have to be resolved to get agreement on this proposal. |
Thanks for correcting me, @annevk. I was biased on my previous statement and I'll fix the wording and make sure we don't skip any steps here. |
@justinfagnani you said the IPR thing was resolved, but it still looks red from here. |
@annevk I created my w3.org account and associated with my Github account. Is there anything more I need to do? I don't have permissions to revalidate the PR. |
c30ed86
to
b907571
Compare
thanks @annevk ! |
See issue #716
I've tried to incorporate feedback from the issue, last F2F, and reviews from colleagues.