Fix observations #101
Fix observations #101
Conversation
|
cc @jackfranklin @ChristianMurphy @PixelsCommander since you all chimed into the previous PR #100 |
| const observer = new MutationObserver(() => { | ||
| ReactDOM.unmountComponentAtNode(getRenderRoot(self, useShadowDom)); | ||
| const props = utils.getProps(self); | ||
| props.children = utils.getChildren(self); |
There was a problem hiding this comment.
can we get rid of utils.getChildren now ? :)
There was a problem hiding this comment.
No, it's used in the initial render (where we do want it to be destructive, as we're about to move the DOM elements from the current DOM node they're in, to React for it to render)
I think very possibly getChildren as a name could stand to be improved, though, to make its destructive nature a bit clearer. Open to naming suggestions there, though
|
@stephencookdev this is a really smart solution! I'll let @ChristianMurphy chime in but to me this looks 👌 |
| // as this will cause a mutation in the current React component (because | ||
| // of the way that DOM nodes must be unique) | ||
| // but the fragment is a reference, so this is fine to keep a hold of | ||
| props.children = this.childrenFragment; |
There was a problem hiding this comment.
The mutation observer also watches for changes to the slot/children/subtree.
Caching the childrenFragment will prevent updates that add children to the slot from being rendered.
🤨 The point of wrapping a React components as a web component is to be able to use it outside React. |
I'm a bit confused by this comment. The point of React is that it owns the DOM and rendering your components into it. @ChristianMurphy can you maybe show an example of when you need it to work with some outside library manipulating a component? We're struggling to understand because we don't do that, we use this library to render React components as web elements. All the state/DOM logic/etc is contained within those React components. |
On a similar note, I recently made this shadow-root feature optional - but before my change to make it optional, then all children of the web component would be rendered into a shadow DOM. This means that, before my change, component children would have been entirely hidden from other libraries. I'm unclear as to how my change here is less permissive than that? |
Children get rendered into the shadow DOM, but content can still be updated through the slot.
That is the point of React.
TL;DR
Long version: The second motivation for wanting this feature, is keeping inline with the web component capabilities of Angular and Vue. |
|
(closing this, since there's no plan for merging this, as per comment thread) |
Currently we call
getChildrenon both the creation, and change observation of nodes.Unfortunately,
getChildrenis actually destructive, as it pulls child elements into a document fragment, and DOM nodes must be unique - causing a mutation of the underlying React container, causing React to get super upset and exploding.This PR simply does not recreate the document fragments on observed changes. Since the document fragment itself is just a reference to the children, then it should be fine to keep a hold to the same fragment forever (regardless of changes to the underlying children)
The only case this wouldn't be true is if someone were to attempt adding new nodes to the child list outside of React. But I can't see that this would ever be a good idea (rather than using React for this), nor can I see any way of making React not be hugely upset by this (given that you would be effectively changing React's rendered output manually from outside React, which React does get super upset about)