react-reverse-portal icon indicating copy to clipboard operation
react-reverse-portal copied to clipboard

Some non-hook function results are not preserved after conditional OutPortal

Open paulsohn opened this issue 2 years ago • 2 comments

I would like to

  • transfer components which render results depend on nondeterministic, direct function call(such as Math.random() or Date.now()) and
  • fix the results before and after transfer, without storing them internally
  • OutPortal for the components are conditional

And for these purposes, it seems like this package doesn't do what I expected.


Here's the MWE example: CodeSandbox Link

//Box.tsx
function Box({ txt }: BoxProps) {
  return (
    <div style={{ border: "1px solid red" }}>
      <strong>{txt}</strong>
      <br />
      {Date.now()}
    </div>
  );
}
export const MemoBox = memo(Box);
//App.tsx
import { MemoBox } from "./Box";

function App() {
  const [show, setShow] = useState(true);
  const node1 = useMemo(createHtmlPortalNode, []);
  const node2 = useMemo(createHtmlPortalNode, []);
  const toggle = useCallback(() => { setShow((show) => !show); }, []);

  return (
    <>
      <InPortal node={node1}> <MemoBox txt="111" /> </InPortal>
      <InPortal node={node2}> <MemoBox txt="222" /> </InPortal>
      <button onClick={toggle}>Click me!</button>

      <div>
        {show && <OutPortal node={node1} />}
        {show && <OutPortal node={node2} />}
      </div>
      <div>
        {!show && <OutPortal node={node1} />}
      </div>
    </>
  );
}

Every time I toggle, Box "111" alters its position and Box "222" repeats hiding and showing. Now let's focus on the timestamp. Box "111" prints with the fixed timestamp, because every render exposes either OutPortal node1. This is fine. Box "222" however, the timestamp has changed on every show, and this behavior is not quite intuitive when the package description says:

Rendering to zero OutPortals is fine: the node will be rendered as normal, using just the props provided inside the InPortal definition, and will continue to happily exist but just not appear in the DOM anywhere.

rather, it looks like when OutPortal node2 is absent, the detached node2 has been garbage collected and previous timestamp is gone forever.

I'm aware that below solutions will fix Box "222" timestamp as well, so that my purposes are satisfied:

  • modifying the Box implementation (namely using useEffect and useState) to store the timestamp right after mounting
  • or even simpler, insert !show && OutPortal node2 as well but wrapped inside a display:none div.

yet I'm curious why did the timestamp change when the component is supposed to be memoized - given that without these portals React.memo() will do its work and fix the whole results.

paulsohn avatar Sep 17 '22 11:09 paulsohn

Huh, interesting, thanks for reporting this @paulsohn!

it looks like when OutPortal node2 is absent, the detached node2 has been garbage collected and previous timestamp is gone forever.

I don't think that's correct. This is showing that React is doing a re-render for this component, but that doesn't mean the DOM nodes are otherwise affected, they're relatively independent processes. You can see that the DOM nodes aren't garbage collected when removing from the page by testing something where the DOM itself has state (rather than React), e.g. remove and re-add a playing <video> element or a form containing input.

That doesn't help though, because I'm not sure why React is re-rendering to update the value here! That suggests that it thinks the props for this MemoBox have been updated during this process for some reason... Any ideas?

I think that React.memo() is only a shallow comparison over all props, so I suspect this is happening because of some prop update we're doing that means that a prop has a top-level change (e.g. a cloned array or object) even though the internal values haven't changed. Not sure where exactly though.

You might be able to find out more digging into this with the React devtools, which will tell you exactly why each component re-renders.

If the above is correct, you can also probably fix it by passing a 2nd argument to React.memo(), to define exactly what you're memoizing against. In this case, instead of a shallow comparison of all props, what you really want is something like (oldProps, newProps) => oldProps.txt === newProps.txt (i.e. props only count as different if txt changes). If you test that, do let me know if it works, that'd be a strong sign that some props equivalent-but-not-technically-equal update is the cause.

pimterry avatar Sep 21 '22 12:09 pimterry

Thanks for your comment! Hotfixing with export const MemoBox = memo(Box, isEqual) didn't helped me though - the props are already shallow enough.

BTW I'm currently working with my toy project using this awesome package and need to implement this kind of pattern - and that project doesn't suffer from this issue. This makes me more curious than ever... hopefully I can get back after I figured out what's the difference.

paulsohn avatar Sep 29 '22 09:09 paulsohn