facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.86k stars 46.51k forks source link

Support for changing a Portal's container without remounting children? #12247

Closed danielran closed 4 years ago

danielran commented 6 years ago

(This is related to https://github.com/facebook/react/issues/3965.)

I'm working on a project with a lot of globally unique component instances. (In other words, their keys are essentially database IDs.) It also has DnD functionality. For reordering, everything is fine, but moving an instance from one parent to another causes a complete re-render of the instance.

Instead of moving the nodes around myself, I was thinking of using Portals. Each instance has a prop for which element ID they should render inside. Then, to reparent them, it'd be a simple case of passing a different element ID. (I'm using Redux, so it'd just be a string in the state.)

However, changing a Portal's container node also causes a complete re-render of everything passed to ReactDOM.createPortal. (See this CodePen.)

Would it be possible to skip this re-rendering and effectively move the Portal contents instead?

alexreardon commented 6 years ago

We are also hitting against this issue in react-beautiful-dnd: https://github.com/atlassian/react-beautiful-dnd/issues/192

alexreardon commented 6 years ago

We have found that moving a component to a portal, especially with lots of children, has a significant performance penalty

gaearon commented 6 years ago

@alexreardon Can you describe in more detail what you're trying to do with moving portals? It's not entirely clear to me. Maybe some examples of the tree before and after?

dantman commented 6 years ago

This sounds like the same issue React has in general with when you want to move things from one parent to another. The presence of portals makes this novelly interesting, the DOM element is not one React created but one the user gave to react. In fact this sounds the exact same, because the portal interface isn't one you do something like new Portal but ReactDOM.createPortal(child, container) where container is a dom node and child is a ReactElement tree. child is new on every single createPortal call. So of course container is the "key" here and the only way React knows if it can modify/move things or if it has to render from scratch.

The issue for reparenting is https://github.com/facebook/react/issues/3965 and has been an issue for nearly 3 years.

Recently I started an RFC for a possible reparenting API: https://github.com/reactjs/rfcs/pull/34

This API should work with Portals as well. You can myReparent = React.createReparent(this); in your class to create a reparent. Then use React.createPortal(this.myReparent(<div>...</div>), document.getElementById('root-' + this.state.root) in render. When container changes the contents of the reparent will be moved to the new DOM node the same as they would be when they are moved to another parent in the React tree.

alexreardon commented 6 years ago

I will give a bit more context @gaearon.

When a user starts a drag interaction we use position:fixed on the dragging item to control its position. Currently we leave the element in the same spot in the component tree. However, this has style issues as position:fixed does not play well with parents that have a transform on them.

As a part of our React 16 upgrade we are hoping to move the dragging item into a React portal so that it does not matter what styles your parents have (including transform).

We are doing this:

if (!shouldUsePortal) {
  // For the purpose of testing I kept the structure the same for both render paths
      return (
        <React.Fragment>
          {child}
          {null}
        </React.Fragment>
      );
}

// When dragging we put the Draggable into a portal
const inPortal: Portal = ReactDOM.createPortal(
  child,
  // simply gets our portal dom element
  getPortal(),
);

return (
  <React.Fragment>
    {inPortal}
    <Placeholder placeholder={dimension.placeholder} />
  </React.Fragment>
);

When the component is rendered into a portal it unmounts the component and mounts it in the new location. We are finding that this has quite a large cost. This cost is extremely high when the dragging item being but into a portal has a lot of children.

alexreardon commented 6 years ago

I think #12454 is a related concern to your reparenting discussion @dantman

thysultan commented 6 years ago

So of course container is the "key" here and the only way React knows if it can modify/move things or if it has to render from scratch.

If i'm not mistaken the createPortal has a third optional argument key that can be used for this – createPortal(element, container, key).

The only reason i understand that react does this is the same reason that two elements with differing types i.e <A/> and <B/> replace each other.

The behavior suggested by @danielran looks like a reasonable way to solve this pattern, albeit it might be a breaking change if the current heuristic is expected/documented behavior.

dantman commented 6 years ago

If i'm not mistaken the createPortal has a third optional argument key that can be used for this – createPortal(element, container, key).

The only reason i understand that react does this is the same reason that two elements with differing types i.e <A/> and <B/> replace each other.

Correct, there is a (not well documented) key argument to createPortal, this is simply passed as the key prop to the resulting React element. This is likely just so that you can use it in an array. i.e. dialogs.map(dialog => React.createPortal(<Dialog {...dialog} />, getDialogContainerFor(dialog.id), dialog.id)). I should've used a term other than key to refer to the globally unique identity by which React understands that two portals are the same.


Though I suppose in this case React does have a bit more information than just child and container. The portal is a normal react element with a local key and it is rendered into a parent in react the same way a normal <MyComponent /> would be. So react knows whether a portal is in the same slot. And that would be enough information to know what the previous portal in that slot was and identify that the container value changed, rather than it being a separate portal in another part of the react tree.

Although, while that's true one could argue that saying const container = someState ? containerA : containerB; return React.createPortal(<div />, container); is somewhat similar to saying const Component = someState ? ComponentA : ComponentB; return <Component><div /></Component>; and shouldn't actually try and re-use the state tree of the possibly unrelated portals.

In fact I can think of a possible deoptimization as a side effect of this:

if ( isA ) {
  return React.createPortal(<div className='foo-a'>...some complex page...<A /></div>, containerA);
} else {
  return React.createPortal(<div className='bar-b' style={style}>...some complex page...<B /></div>, containerB);
}

Current likely behaviour when going from isA to not-isA:

Behaviour for createPortal calls without use of the undocumented key arg, after making it so that changing container moves the tree instead of replacing it:


And while we could technically handle this, in the very specific case where you render a portal and then change the container of the portal, I'm not sure it's at all useful to implement. It only covers a single use case, it doesn't help with the (IMHO likely more common) desire of being able to move things in/out of a portal, and unlike the other use cases it doesn't solve it is trivial to workaround in user-space.

The OP suggestion seems to be to optimize this general idea:

// html
<div id="rootA"></div>
<div id="rootB"></div>
// render
React.createPortal(<div class="container"><Foo /></div>, condition ? rootA : rootB);

So that when rootA becomes rootB is changed React does a bunch of work to move the dom nodes. However the general idea of Portals is to ask React to do less and let you control the dom outside of React instead of asking React to do more dom work. As you control the dom that the portal is rendered into, you can trivially change this general idea to instead work like this.

// html
<div id="rootA"></div>
<div id="rootB"></div>
// mount
this.setState({container: document.createElement('div')});
// update
(condition ? rootA : rootB).appendChild(state.container);
// render
React.createPortal(<Foo />, state.container);

And it requires no work in React or possible negative side effects in order to get things to working. And we can focus on the larger scope of reparenting issues.

benwiley4000 commented 5 years ago

I'd like to pile on another pretty specific use case that would be very nice to solve with portals, since currently I have to do a whole bunch of manual DOM wrangling I'd like to not do.

Basically, I'm rendering a hidden video element at the top of my element hierarchy, and at different points I want to be able to display the video in different positions on the page. Due the the highly logic-coupled-to-view nature of the HTMLVideoElement I have to physically move the same element around the page in order to experience minimal disruption to the media playback state.

Similar to @alexreardon I don't want to be forced to rely on style-based repositioning as I'm building a library and asking users to use absolute positioning to solve their layout problems seems wrong. So for me it's a requirement that I can move the same element around to different places in the DOM (moving the element will pause my video forcing me to call .play() again, but if I plan this transition it can be mostly seamless).

So why do I feel that I want portals to solve this problem for me? Well if not, then my only choice is to render my <video> in its own component, set shouldComponentUpdate to false, and handle all DOM attribute and event handler updates (as well as moving to different parents) manually on UNSAFE_componentWillReceiveProps.

Does anyone know a workaround?

benwiley4000 commented 5 years ago

@gaearon would you accept a pull request for this?

mmartinson commented 5 years ago

@benwiley4000 I'm doing some research to solve a similar problem. Have you made any progress on this? Has your workaround been effective?

gaearon commented 5 years ago

We had a similar problem and solved it in userland with a combination of portals, context, and refs. It's a bit convoluted but it works. The idea is that you have state and a "node manager" live at the top. Your leaf component, when it mounts, passes its "leaf div" up the context to the "node manager". In return, the "node manager" gives the leaf component a node to portal into. That node would always be the same. Node manager would create it once. Initially, node manager would attach it to the leaf's own "leaf div". But it can also detach it and attach it somewhere else without recreating the element. From leaf's point of view, it always renders the portal into the same node. But the thing at the top can move that node from an actual div (originally in the leaf) to any other div on the page. Instead of reparenting the portal itself, we always portal into the same node, but that node itself is moved in the document imperatively wherever we need.

benwiley4000 commented 5 years ago

@mmartinson there's no real "workaround" except to not use React for this. I'm doing stuff slightly differently than described above.. but basically I render a div container in a component somewhere, then call this callback with a ref to the div go have the video rendered there. Another video container can steal it later if wanted. I'm doing all of that with raw DOM.. as far as React is concerned, the video element doesn't exist, but something convenient I learned was that React won't mess with children manually-added to empty elements, even on re-render. So no need for the shouldComponentUpdate guard I had discussed before.

gaearon commented 5 years ago

There is a workaround — I just described it. We use it, and it works. The description is a little brief but I can try to create a fiddle if someone gives me a base to work on.

gaearon commented 5 years ago

Oh wait, I guess you described roughly the same thing.

benwiley4000 commented 5 years ago

@gaearon hm what's the advantage of using the portal here instead of just appendChild? I guess that makes sense for a more complex bit of markup you'd want to move around. Is there any code you could share?

benwiley4000 commented 5 years ago

Our conversation has a bit of a race condition going on 😄. Yes I'd love to see some example code!

gaearon commented 5 years ago

Ping @sompylasar, I think you had some demo that wasn't using any proprietary code?

sompylasar commented 5 years ago

@gaearon Using React.Portal? No, I don't think so.

mmartinson commented 5 years ago

I think I've got it working. Thanks all for the input.

benwiley4000 commented 5 years ago

@mmartinson using portals?

benwiley4000 commented 5 years ago

@mmartinson hey, just wanted to poke again on this.. did the solution you found use portals?

mmartinson commented 5 years ago

Hey @benwiley4000. Ya the solution I found does use portals, though I'm not sure whether the use is superfluous or not with the other imperative changes that are being done. My understanding of the internal mechanics are admittedly limited.

I have a hidden media manager component with ref that is passed to the media component when it renders. Media component is rendered with React.forwardRef, then calls ReactDOM.createPortal with that ref. There is a intermediate wrapper div between the media manger and media component.

To change position, the media manager uses the props in UNSAFE_componentWillReceiveProps to determine the required position, then does DOM element lookups by id to remove the existing media element with oldTargetElem.removeChild(oldTargetElem.firstChild), and then adds it to the new target location with:

const newTargetElem =
  document.getElementById(`${newPosition}-media-container`) ||
  document.getElementById('hidden-media-container');

const Media = this.mediaWrapperRef.current;
if (!Media || !newTargetElem) return;

newTargetElem.appendChild(Media);

This seems to do it for me, though I'm not certain if there are any race conditions lurkers in there that I'm just getting lucky with.

pimterry commented 5 years ago

@gaearon @benwiley4000 I hit this too & fixed it for myself. Rather than put together an example, I've turned the basic pattern you're describing into a library you can use to solve this properly.

It's effectively the concept defined above, with a few additions, and lets you define some content in one place, render & mount it there once, then place it elsewhere and move it later (potentially out of the page entirely, and back) all without remounting or necessarily rerendering.

I'm calling it reverse portals, since it's the opposite model to normal portals: instead of pushing content from a React component to distant DOM, you define content in one place, then declaratively pull that DOM into your React tree elsewhere. I'm using it in production to reparent expensive-to-initialize components, it's working very nicely for me!

Repo is over here: https://github.com/httptoolkit/react-reverse-portal. Let me know if it works for you!

diego-betto commented 4 years ago

In my case I only needed to update the component on url and component change basis, so I used key={component.id + window.location.path}

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

stale[bot] commented 4 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

stale[bot] commented 4 years ago

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!