Skip to content
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

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

Closed
danielran opened this issue Feb 19, 2018 · 28 comments
Closed

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

danielran opened this issue Feb 19, 2018 · 28 comments
Labels

Comments

@danielran
Copy link

(This is related to #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
Copy link

We are also hitting against this issue in react-beautiful-dnd: atlassian/react-beautiful-dnd#192

@alexreardon
Copy link

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

@gaearon
Copy link
Collaborator

gaearon commented Mar 28, 2018

@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
Copy link
Contributor

dantman commented Mar 28, 2018

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 #3965 and has been an issue for nearly 3 years.

Recently I started an RFC for a possible reparenting API: reactjs/rfcs#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
Copy link

alexreardon commented Mar 28, 2018

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
Copy link

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

@thysultan
Copy link

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
Copy link
Contributor

dantman commented Mar 28, 2018

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:

  • React unmounts A and removes .foo-a and its contents
  • React renders .bar-b and mounts B

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:

  • React unmounts A
  • React moves .foo-a to containerB
  • React does a virtual diff of the dom, and queues up a bunch of dom mutations to change the className of the div, mutate any elements in ...some complex page... that React thinks look similar enough, and add/remove elements that individually are only part of either .foo-a or .bar-b.
  • React mounts B

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
Copy link

benwiley4000 commented Nov 28, 2018

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
Copy link

@gaearon would you accept a pull request for this?

@mmartinson
Copy link

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

@gaearon
Copy link
Collaborator

gaearon commented Apr 24, 2019

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
Copy link

@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
Copy link
Collaborator

gaearon commented Apr 24, 2019

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
Copy link
Collaborator

gaearon commented Apr 24, 2019

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

@benwiley4000
Copy link

@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
Copy link

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

@gaearon
Copy link
Collaborator

gaearon commented Apr 24, 2019

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

@sompylasar
Copy link
Contributor

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

@mmartinson
Copy link

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

@benwiley4000
Copy link

@mmartinson using portals?

@benwiley4000
Copy link

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

@mmartinson
Copy link

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
Copy link

@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
Copy link

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
Copy link

stale bot commented Mar 11, 2020

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 stale bot added the Resolution: Stale Automatically closed due to inactivity label Mar 11, 2020
@stale
Copy link

stale bot commented Mar 18, 2020

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 stale bot closed this as completed Mar 18, 2020
@stale
Copy link

stale bot commented Mar 26, 2020

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants