[labs/ssr-dom-shim] Add basic support for element internals #3677
Conversation
🦋 Changeset detectedLatest commit: 056a39c The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Let me know if you'd like me to revert the package-lock as well. |
|
@justinfagnani it looks like I checked in some build files, is that normal for Lit or should I remove those? |
|
@calebdwilliams awesome!
Those should be added to |
|
And yeah, let's revert the package-lock change, since there's no package.json change. We've been trying to run on very recent versions of npm to keep the package-lock stable, btw. |
Also with regards to this, we need to add to wireit configs. @calebdwilliams I'm happy to handle any of our monorepo consistency related changes myself and push it up if you'd like. |
|
I updated the PR to remove changes to the package-lock and removed the compiled files. |
calebdwilliams
requested review from
augustjk
and removed request for
aomarks,
justinfagnani and
augustjk
|
I apparently somehow removed @aomarks and @justinfagnani as reviewers. Not sure how that happened, sorry about that. |
|
|
||
| /** | ||
| * @TODO | ||
| * - This can be typed better with keyof ARIAMixin, but TypeScript's definition |
There was a problem hiding this comment.
It would be great to have some reference links here. One to a Typescript DOM lib issue, another to the source of this list.
There was a problem hiding this comment.
Looks like all of the browsers currently support a different set of features. All of them do reflect the from AriaMixin, so would it just be better in the long run to keep this list to Record<keyof AriaMixin, string>?
There was a problem hiding this comment.
That sounds reasonable
| role: 'role', | ||
| }; | ||
|
|
||
| /** Force the attributes onto the reference element based on internals properties */ |
There was a problem hiding this comment.
I'm not sure I understand the purpose of this method from this description. Is this something not part of AOM? When would we call it?
There was a problem hiding this comment.
Does the new description help this?
| constructor(_host: InternalsHost) { | ||
| this._host = _host; | ||
|
|
||
| initAom(_host, this); |
There was a problem hiding this comment.
native AOM doesn't automatically reflect to attributes, does it? I think we should probably leave behavior like this up to ElementRenderers like LitElementRenderer.
There was a problem hiding this comment.
It doesn't if you use internals. The purpose here is to hydrate that behavior. If we were following the semantics, we should check to see if the attributes are set, if they aren't, append it to the element for SEO/SSR purposes then remove the element upon hydration if it still matches.
I think having the AOM stuff reflected from internals to the pre-hydrated code would be a huge win for capturing the semantics of an element and for the DX of authoring components using internals/Lit SSR.
| initAom(_host, this); | ||
| } | ||
| checkValidity() { | ||
| return true; |
There was a problem hiding this comment.
Do we need to implement this one? I'm not sure, but seems to me like we might want to throw rather than return a wrong answer. If we want to provide a correct answer, that would take some work, yeah?
There was a problem hiding this comment.
Yeah, it would take some work for sure. We could just throw an error on the methods or just noop them …
I think the bigger question would be for things like setFormValue and setValidity, those should exist but only be side-effectful upon hydration.
calebdwilliams
requested review from
justinfagnani and
augustjk
and removed request for
aomarks and
justinfagnani
| * is created on an element to wire up the getters/setters | ||
| * for the AriaMixin properties | ||
| * | ||
| * TODO - Determine the proper way to hydrate any attributes set by the shim |
There was a problem hiding this comment.
@justinfagnani Following up on your question from the other day. I'm happy to reference an issue, but I'm not sure where to put that. The version of TypeScript I'm running in VSCode matches the spec, but neither Chromium or Webkit (or Firefox with the ARIA reflection tag) have the same tags. The ones listed here are in line with Chromium's implementation.
To complicate that a bit more, the version of TypeScript building Lit's repo is different from the version running in VSCode (somehow) so that it recognizes different properties on ARIAMixin which appears to be somewhat volatile in TypeScript.
Any guidance you could provide on how to add more clarity here would be welcomed.
* Add a rough implementation for ElementShim.prototype.attachInternals Fixes lit#3676
- Put element-internals file under lib/ and export out of index - Augment ARIAMixin interface - Export const for hydrate-internals- prefix
augustjk
force-pushed
the
feat/ssr-dom-shim-internals
branch
from
758469a
to
dcef2b4
Compare
| // created on an element to wire up the getters/setters for the ARIAMixin | ||
| // properties. | ||
| const internals = ( | ||
| this.element as object as {__internals: ElementInternals} |
There was a problem hiding this comment.
So this only work when we're using our shim. We need to think about what happens if someone loads jsdom. We may need adapters, like a getInternals() callback.
There was a problem hiding this comment.
I'm not sure if there's anything to be done right now. Neither jsdom nor happy-dom have attachInternals() implemented and a canonical way of getting the internals object from an element doesn't exist.
Do you mean a getInternals() on the element renderer? This would be more of a DOM shim dependent thing rather than an element renderer thing.
There was a problem hiding this comment.
We should have an issue somewhere for using another DOM shim, and this can go on the list of things we'll need to consider there. What I mean by getInternals() is that since there's no standard DOM API for getting at that from outside a component, we'll need a callback per DOM emulation library to get at it. Future work, just noting it.
- Throw on repeat call to `attachInternals()`. - Add warning for `checkValidity()` calls. - Retype and rename `ariaMixinEnum` to `ariaMixinAttributes`. - Loop through ariaAttrMap instead of internals instance.
To do