fix(synthetic-shadow): use native mutation observer for portal elements #1596
Conversation
| @@ -25,15 +30,21 @@ let portalObserver: MutationObserver | undefined; | |||
|
|
|||
| const portalObserverConfig: MutationObserverInit = { | |||
| childList: true, | |||
| subtree: true, | |||
| subtree: false, | |||
There was a problem hiding this comment.
this is false by default, just remove it.
| return; // nothing to do here, it is already correctly patched | ||
| } | ||
| setShadowRootResolver(node, fn); | ||
| if (node instanceof Element) { | ||
| // Root LWC elements can't contain slots, therefore we stop listening mutations. |
There was a problem hiding this comment.
the comment is wrong... "Root LWC elements can't get content slotted into them, therefore we don't observe their children."
| if (isUndefined(previousNodeShadowResolver)) { | ||
| // we only care about Element without shadowResolver (no MO.observe has been called) | ||
| MutationObserverObserve.call(portalObserver, node, portalObserverConfig); | ||
| } | ||
| setShadowToken(node, shadowToken); |
There was a problem hiding this comment.
@caridy why do we use this (other than to propagate in the portal elements)? This code path is not adding it to the host elements.
There was a problem hiding this comment.
this has to be accommodated, it should be added to custom elements, but not to the children elements of a it. I think we can keep the old condition if (node instanceof Element) {, then do the setShadowtoken(), then checking if it is not host to do the rest, which is install mutation and attempt to recursively patch all children.
elements marked as lwc:dom=manual. It also adds a performance optimization by not listening to the whole subtree.
jodarove
force-pushed
the
jodarove/improve-mo-for-dom-manual
branch
from
3efdc6a to
dc98cf6
Compare
| if (isUndefined(previousNodeShadowResolver)) { | ||
| // we only care about Element without shadowResolver (no MO.observe has been called) | ||
| MutationObserverObserve.call(portalObserver, node, portalObserverConfig); | ||
| } | ||
| // recursively patching all children as well | ||
| const childNodes = getInternalChildNodes(node); |
There was a problem hiding this comment.
@caridy, @ravijayaramappa since for portals we don't observe mutations on LWC components, what do you think about using the native apis? (childNodesGetter)
There was a problem hiding this comment.
Oh, that's a great idea. Since this element is not a host, the only other option is to be a slot, which we don't really care much since we are saying that slots are only good slots if they are coming from template.... we can just use childNodesGetter here to speed things up.
|
Thanks for the contribution! Before we can merge this, we need @jodarove to sign the Salesforce.com Contributor License Agreement. |
| ShadowRootResolver, | ||
| getShadowRootResolver, | ||
| isHostElement, | ||
| } from './shadow-root'; | ||
| import { setShadowToken, getShadowToken } from './shadow-token'; | ||
|
|
||
| const MO = MutationObserver; |
There was a problem hiding this comment.
Avoid caching the MutationObserver in a local const. I had issues where const MO = MutationObserver was resolving the value from the window instead of the imported value.
There was a problem hiding this comment.
I had an issue where the following code would resolve MutationObserver on window in the const assignment. So it would get the polyfilled MO.
import { MutationObserver, MutationObserverObserve } from '../env/mutation-observer';
const MO = MutationObserver;
I cannot repro it anymore. So I was suggesting not to use the const just to be safe. Import the MutationObserver and directly use it instead of assigning it to a const.
There was a problem hiding this comment.
ok, yes... the order might be wrong... better to import it from the cached env file.
ravijayaramappa
changed the title
There was a problem hiding this comment.
LGTM, but we should way for @ravijayaramappa to review this in details.
…ts (#1596) * fix(synthetic-shadow): use native mutation observer for elements marked as lwc:dom=manual. It also adds a performance optimization by not listening to the whole subtree.
elements marked as lwc:dom=manual.
It also adds a performance optimization by not listening to the whole subtree.
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.