OutPortal componentDidUpdate doesn't work

[RusinovAnton]

Hello, thank you for the library, it saves me now.

I'm using it for the audio widget component which should be started at the audio page and then be available for the user while they're browsing through the website.

I had one error which took me some time to figure out how to resolve. I did a deep dive into the component's functionality and wanted to share the insight, so next time someone else experiencing it could save their time.

Error

The issue I've had is that I needed to re-create the node prop based on the id I have, but OutPortal's componentDidMount was failing because of the error being thrown at the .mount() method:

Cannot read property 'replaceChild' of null

https://github.com/httptoolkit/react-reverse-portal/blob/master/src/index.tsx#L47-L50

Usage

So here is how I was using the react-reverse-portal:

const MyComponent = () => {
  const { setPortalNode } = usePortalNodeReference();
  const { Id, asWidget } = useDataState();
  const portalNode = useMemo(() => createPortalNode(), [Id]);
  setPortalNode(portalNode);

 return  (
    <>
      <InPortal node={portalNode}><AudioComponent /></InPortal>
      {asWidget &&  <OutPortal name="widget" node={portalNode} Id={Id}></OutPortal>}
    </>
  );
}

const AnotherComponent = () => {
  const { Id, asWidget } = useDataState();
  const { portalNode } = usePortalNodeReference();
  return !asWidget && <OutPortal name="page" node={portalNode}/>
}

Every time, portalNode or Id prop was updating, OutPortal would call the .mount() and it would throw an error.

I debugged it and this is because OutPortal's placeholder reference is missing the parentNode once .replaceChild was called on it, and then after the update, it tries to use the same placeholder's parentNode again in the mount assuming that it is present:

https://github.com/httptoolkit/react-reverse-portal/blob/master/src/index.tsx#L148

I cannot tell why exactly the placeholderNode doesn't get re-rendered on prop change and get a parent node again, but I feel is because of .replaceChild() method call at .mount() is breaking react's virtual dom representation.

At the end of the day I came to the solution where I do not re-create portalNode, but use key prop to force re-mount OutPortal and AudioComponent when the Id prop changes:

const MyComponent = () => {
  const { setPortalNode } = usePortalNodeReference();
  const { Id, asWidget } = useDataState();
-  const portalNode = useMemo(() => createPortalNode(), [Id]);
+  const portalNode = useMemo(() => createPortalNode(), []);
  setPortalNode(portalNode);

 return  (
    <>
-      <InPortal node={portalNode}><AudioComponent /></InPortal>
+     <InPortal node={portalNode}><AudioComponent key={Id} /></InPortal>
-      {asWidget &&  <OutPortal name="widget" node={portalNode} Id={Id}></OutPortal>}
+     {asWidget &&  <OutPortal key={Id} name="widget" node={portalNode} Id={Id}></OutPortal>}
    </>
  );
}

Outcome

Having all of this information, I think it is sensible to have a check for placeholders parent node before using it, also react-reverse-portal could give component's user a warning with an explanation of what is happening.

[RusinovAnton]

I hope some of this text makes sense, if none, I'm here to answer your questions

[pimterry]

That's very interesting, thanks. It's a little more complicated that this I think, but this makes sense.

I cannot tell why exactly the placeholderNode doesn't get re-rendered on prop change and get a parent node again, but I feel is because of .replaceChild() method call at .mount() is breaking react's virtual dom representation.

This shouldn't matter (unless there's a bug). We don't expect React to put the placeholder back, instead node.unmount() does that. We do change the DOM so it doesn't match the VDOM, but we change it back before the OutPortal is unmounted in all cases, so that any time React tries to change the DOM, the DOM is in the right state.

At the end of the day I came to the solution where I do not re-create portalNode, but use key prop to force re-mount OutPortal and AudioComponent when the Id prop changes

This is the key I think. The node itself manages the DOM, but by recreating the node, you end up with two nodes managing one OutPortal. I think that is possible, but it sounds like we don't do it correctly at the moment.

It sounds like the flow of what's happening here is:

• The OutPortal is mounted with Node A
• On mount, Node A removes the placeholder, and places its own content
• Node A also remembers the placeholder and its previous parent, so it can put it back on node.unmount()
• The OutPortal is updated, with Node B
• Node B then tries to replace the placeholder, panic! Nobody has talked to Node A, who is currently using this, so it explodes.

Currently, we do handle the case where you have two active nodes like this, but we expect that they don't have the same OutPortal, so something calls node.unmount() at some point, rather than just calling node.mount() twice.

I think this is fixable. OutPortal needs to keep track of its last node prop (as a simple field, no setState required) and to then unmount the previous node before updating, if that prop has changed.

That should make this work. If you'd like, you're welcome to take a look at this and open a PR? If not, I should have time within the next week or two.

[RusinovAnton]

I'll try to look at it this week but cannot promise anything 😬

[pimterry]

Sounds good! Let me know if you have any questions or if there's anything I can do to help.

[pimterry]

Hey @RusinovAnton, I think I've just fixed this in 85ada76. Can you test it out, and let me know if it works for you?

[spautz]

Not the reporter, but I ran into this same issue and 85ada76 fixed it for me. Thank you!

As a minor aside: it might be slightly easier to test out specific commits if this repo's package.json had a prepare script which ran the build, because then consumers could install like github:httptoolkit/react-reverse-portal#85ada76 instead of running the build manually. Happy to submit a PR if that's desired.

[pimterry]

Thanks for the reminder & testing @spautz, I completely forgot to chase this up! If you do have a second, I would happily accept a PR to improve the git install flow too, if that'd be useful to you.

Anyway, I've done some more testing myself too, I'm happy this is all working nicely now. Fix now released, as v1.0.5. Thanks everybody!