Support comment node as a mount point #9835
Conversation
sophiebits
force-pushed
the
comment-mount-point
branch
from
2a33c90
to
7faca34
Compare
|
This was surprisingly a lot easier than expected. What do you think? cc @pieterv @sebmarkbage |
| @@ -347,34 +354,49 @@ var DOMRenderer = ReactFiberReconciler({ | |||
| parentInstance: Instance | Container, | |||
| child: Instance | TextInstance, | |||
| ): void { | |||
| parentInstance.appendChild(child); | |||
| if (parentInstance.nodeType === COMMENT_NODE) { | |||
There was a problem hiding this comment.
This is the check I was worried about. I wonder if we should have a different set of helpers that only apply when parentInstance is Container, i.e. the parent host fiber is a HostRoot so that this check only needs to apply on the very top level.
This check probably isn't that slow but it is for every insertion and we want that to be super fast if we for example go through this path for optimized paths that creates DOM nodes from server rendered markup.
There was a problem hiding this comment.
Hm. Maybe it's not so bad since markup generators would likely use the appendInitialChild path instead and not this one.
Still, seems cleaner if the Container had its own helpers so that hosts don't need to have to build pattern matching into their data structures when we already have the HostRoot tag for that.
There was a problem hiding this comment.
If it's a hot path, could checking for a non-getter property help here?
parentInstance.insertData !== undefined
There was a problem hiding this comment.
Let's branch in commitWork based on if the parent host fiber is HostRoot instead. That will avoid the check in the host config so that we can use it in hot paths where the check isn't needed. It will also let us get rid of this awkwardness in the RN renderer: https://github.com/facebook/react/blob/master/src/renderers/native/ReactNativeFiberRenderer.js#L62-L70
|
Love the general API. Only additional thing i would want is to provide a way to remove some placeholder content on commit, your idea (from when we chatted offline) of having start and end comment nodes sounds good to me since it would allow multiple nodes which works well for my usecase. I'm happy to do any work here to get this over the line so let me know if you want any help :) |
|
We currently remove placeholder content inside a container before rendering. It's a bit awkward since with async the placeholder gets removed synchronously. I think to do that, we'd probably need to tie into the hydration code which will need to know whether we want to hydrate the existing content or always remove it. |
This makes it so you don't need to pattern-match manually to build a renderer where the container and instance types are not the same. Prerequisite to facebook#9835.
This makes it so you don't need to pattern-match manually to build a renderer where the container and instance types are not the same. Prerequisite to facebook#9835.
This makes it so you don't need to pattern-match manually to build a renderer where the container and instance types are not the same. Prerequisite to facebook#9835.
This makes it so you don't need to pattern-match manually to build a renderer where the container and instance types are not the same. Prerequisite to facebook#9835.
This makes it so you don't need to pattern-match manually to build a renderer where the container and instance types are not the same. Prerequisite to #9835.
sophiebits
force-pushed
the
comment-mount-point
branch
3 times, most recently
from
d0fa63a
to
d3186de
Compare
sophiebits
force-pushed
the
comment-mount-point
branch
from
d3186de
to
ef496c4
Compare
| invariant(mountPoint.nodeType === 8, 'Expected comment'); | ||
| }); | ||
|
|
||
| it('renders', () => { |
There was a problem hiding this comment.
Better test description plz.
There was a problem hiding this comment.
It's in a describe tho! Not my fault that it doesn't show up nicely in the file. (Okay, yes it is…)
sophiebits
force-pushed
the
comment-mount-point
branch
from
ef496c4
to
2cfa3a8
Compare
This means you don't need an extra wrapper div for each component root you need. Rendered stuff is inserted before the comment you pass in.
sophiebits
force-pushed
the
comment-mount-point
branch
from
2cfa3a8
to
e09e60d
Compare
| container.nodeType !== 1 || | ||
| !container.tagName || | ||
| container.tagName.toUpperCase() !== 'BODY', | ||
| container.nodeType !== ELEMENT_NODE || |
There was a problem hiding this comment.
Thanks for updating the node types to use constants.
| 'Aaeiou<!-- react-mount-point-unstable -->B', | ||
| ); | ||
|
|
||
| ReactDOM.render(list('yea'), mountPoint); |
There was a problem hiding this comment.
Why have both this and the previous expectation? They seem to be testing the same thing.
There was a problem hiding this comment.
This tests deletes and moves (that's why I used keys above).
|
This is really cool! I'd feel a little better about it if the server render/hydration path was tested as well. What do you think about adding a render mode in |
|
Good question. I wouldn't expect this to work server-side right now since we don't know where the React-owned nodes start. I need to look more at how things are set up in order to figure that out. (or if you want to. :)) |
|
Was sorta thinking you might have a comment before and after; then if you have a placeholder React can also handle removing that. |
This means you don't need an extra wrapper div for each component root you need. Rendered stuff is inserted before the comment you pass in.