-
Notifications
You must be signed in to change notification settings - Fork 392
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
feat(engine): enable attachInternals
API
#3670
Conversation
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.
LGTM, very minor nitpicks and suggestions
packages/@lwc/engine-core/src/framework/base-lightning-element.ts
Outdated
Show resolved
Hide resolved
function attachInternals(elm: HTMLElement): ElementInternals { | ||
return HTMLElement.prototype.attachInternals.call(elm); |
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.
function attachInternals(elm: HTMLElement): ElementInternals { | |
return HTMLElement.prototype.attachInternals.call(elm); | |
const attachInternalsFunc = HTMLElement.prototype.attachInternals; | |
function attachInternals(elm: HTMLElement): ElementInternals { | |
return attachInternalsFunc.call(elm); |
I'd feel safer about caching it from the prototype, just in case somebody tries to monkey-patch it later somehow.
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.
@nolanlawson I was thinking about this, I've cached the attachInternals
method as suggested, however, I believe we do want to allow monkey-patching when the method needs to be polyfilled.
Is the intent to load all polyfills prior to loading LWC and prevent further monkey-patching while the engine is running?
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.
Can we add a small test for the shape of ElementInternals
itself? Doesn't need to test the whole API surface (it will vary from browser to browser anyway), but maybe just something to confirm that this.template === this.internals.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.
@nolanlawson I added a few tests to verify the ElementInternals
objects.
I wanted to include some tests for the ARIAMixin
behavior but I'm not sure the tests I added will be useful, could you take a look and let me know what you think?
I'll add some tests for form association + FACE callbacks in a follow-up PR.
BTW I imagine our tests in Safari 14 are going to fail since it doesn't support ElementInternals. I think we'll need to be resilient to browsers that don't support ElementInternals, and throw a helpful error message in that case. (Developers themselves will be responsible for polyfilling as necessary.) |
@nolanlawson I added a check and a test to verify. |
|
||
if (typeof ElementInternals === 'undefined') { | ||
// Browsers that don't support attachInternals will need to be polyfilled before LWC is loaded. | ||
throw new Error('attachInternals API is not supported in this browser environment.'); |
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'd like to note that Jest/JSDOM will throw this error due to jsdom/jsdom#3561 not being merged yet. That's fine; developers will have to support old Safari anyway.
Details
This PR implements the first portion of the
Element Internals and Form Association
RFC by enabling theattachInternals
API.The remaining form association and FACE callbacks will be implemented in a follow-up PR.
This PR also partially resolves #717 and #1023.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-11608302