Initial commit of custom-elements-scoped proof-of-concept #383
Conversation
|
|
||
| Scoped CustomElementRegistry polyfill based on discussion in | ||
| https://github.com/w3c/webcomponents/issues/716 and spec proposal in | ||
| https://github.com/w3c/webcomponents/pull/865. |
There was a problem hiding this comment.
Also link to the latest version at https://github.com/WICG/webcomponents/blob/gh-pages/proposals/Scoped-Custom-Element-Registries.md?
| // Once the registry is patched, this is easy to allow (previously upgraded | ||
| // elements won't be affected, but new elements will be upgraded with the | ||
| // new definition) | ||
| this._allowRedefinition = options && options.allowRedefinition; |
There was a problem hiding this comment.
Removed, per below.
| // Once the registry is patched, this is easy to allow (previously upgraded | ||
| // elements won't be affected, but new elements will be upgraded with the | ||
| // new definition) | ||
| this._allowRedefinition = options && options.allowRedefinition; |
There was a problem hiding this comment.
I don't think I saw redefinition in Justin's proposal -- what's the reason to support it here?
There was a problem hiding this comment.
It was a little gratuitous addition I made while experimenting; basically the implementation makes it easy to allow last-in re-definition. I should probably pull it out, good catch.
| // Helper to patch CE class setAttribute/getAttribute to implement | ||
| // attributeChangedCallback | ||
| const patchAttributes = (elementClass, observedAttributes, attributeChangedCallback) => { | ||
| if (observedAttributes.size && attributeChangedCallback) { |
There was a problem hiding this comment.
nit suggestion: could invert this condition and return, since the whole function body is inside the if.
| if (!isValidScope(scope)) { | ||
| scope = creationContext[creationContext.length-1].getRootNode(); | ||
| if (!isValidScope(scope)) { | ||
| throw new Error('Element is being upgrade outside of a valid tree scope'); |
| } | ||
| adoptedCallback() { | ||
| const definition = definitionForElement.get(this); | ||
| if (definition && definition.adoptedCallback) { |
There was a problem hiding this comment.
definition?.adoptedCallback?.apply
| const awaiting = this._awaitingUpgrade.get(tagName); | ||
| if (awaiting) { | ||
| this._awaitingUpgrade.delete(tagName); | ||
| awaiting.forEach(element => { |
There was a problem hiding this comment.
regular for loop? (or is there some advantage to forEach here?)
| } | ||
| get(tagName) { | ||
| const definition = this._definitions.get(tagName); | ||
| return definition ? definition.elementClass : undefined; |
| @@ -0,0 +1,32 @@ | |||
| { | |||
| "name": "@webcomponents/custom-elements-scoped", | |||
There was a problem hiding this comment.
Maybe scoped-custom-elements? (For some reason custom-elements-scoped to me kinda sounds like it's a variant of the full custom-elements polyfill that also supports scoped). But I don't have a strong opinion.
There was a problem hiding this comment.
👍 Yeah, I take your point. I'm not sure flipping it helps a ton with that particular confusion (i.e. do I need both custom-elements and scoped-custom-elements?). But at a minimum it reads less awkward that way.
There was a problem hiding this comment.
Should we have registry in the name?
@webcomponents/scoped-registry@webcomponents/scoped-elements-registry@webcomponents/scoped-custom-elements-registry
There was a problem hiding this comment.
Changed to scoped-custom-elements-registry
test: add unit test to custom-elements-scoped
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@manolakis Sorry for the delay, thanks so much for the work on this! I got the tests passing now. Could you add the "googlebot I consent" comment, and also review the changes I made in this commit: 825dc92, particularly the change I made to this test, which I think was a mistake in the test, but I may heave mis-understood the intention: 825dc92#diff-b6d5700c31b8bc7d7578717fa944702bb7441e04cfadeeb426fe04db42e45a1dR42 |
|
The algorithm for determining the registry during upgrade is pretty shaky, and I think feedback from this is gonna need to be fed back into the spec proposal. Particularly this case: const registry = new CustomElementRegistry();
registry.define('x-foo', class extends HTMLElement{});
const shadowRoot = document.body.attachShadow({mode: 'open', registry});
const div = shadowRoot.createElement('div');
div.innerHTML = `<x-foo></x-foo>`;Without having stored state on the |
|
@googlebot I consent |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@kevinpschaaf I agree that the test is a bit confusing. My intention was to check what should happen if there is a node in the light DOM which contains an upgraded WC defined in the global registry, and we want to import it into a shadow DOM with a custom registry. What should be the behaviour in this case? Should the instance of the component be of the class defined in the global registry or should be a non upgraded component because that tag name is not defined in the custom registry? I guess we could assume that importing nodes always tries to upgrade them using the current scope. Thoughts? |
|
@kevinpschaaf I'd like to support with some traction here. Are the OP checkboxes up to date? Is there anything specific to take a look? |
|
@leobalter Sorry, haven't been able to focus on this lately. I think this PR is basically ready to land, save for figuring out what we want to do for the test I raised with @manolakis, which I hadn't had a chance to circle back yet. And then yeah, I think the next things would be from that checklist. Switching the code to TS is more of a chore sort of thing, not necessarily critical path but would be nice. Working out polyfill compatibility is probably the more important chunk of work, assuming that's important to your use cases. I expect custom-elements to be fairly straightforward, but ShadyDOM (ShadyCSS) is going to be pretty rough, since right now it scopes element CSS by tag name; the ability to have multiple tag names scoped differently would require adding an additional scoping class/attribute per registry. So I guess a question is... how important is the legacy polyfill story to your use of the scoped registry polyfill, and are you interested in looking at those integrations? |
|
@manolakis Re: the test we were discussing, if we're talking about importing a node, my expectation is that should be purely a function of the node's tagName/attributes and the new context it is being imported into (i.e. whether or not it had a custom element identity before it was imported would be immaterial)... So I could change the test back but make the expectation to be an un-upgrade |
|
@kevinpschaaf Yes, you are right. I tried to clarify the test intention in this PR against this branch #412. All test green 👍 |
test: make test intention clearer
|
@manolakis Right! Sorry, that LGTM, I merged that in. And I'll go ahead and merge this into master, and we can take new PR's from there. |
|
@leobalter For sake of merging this PR, I moved the outstanding TODO's to issues and noted them in the README: |
Initial commit of scoped
CustomElementRegistryproof-of-concept polyfill, to open up community collaboration on moving the polyfill forward (continuation of work done in this gist).Big tasks needed to move this forward (in subsequent PR's):
custom-elementsandshady-dompolyfills===
Edit: remaining TODO's moved to issues: