search

httptoolkit / react-reverse-portal

React reparenting :atom_symbol: Build an element once, move it anywhere
https://httptoolkit.com
Apache License 2.0
889 stars 32 forks source link

Avoid double-rendering InPortal child initially if props aren't overridden #19

Closed bduffany closed 3 years ago

bduffany commented 3 years ago

Bug: The repo's description "build an element once, move it anywhere" is technically incorrect; really it's "build an element twice, move it anywhere." This PR fixes that problem :smiley:

Before: https://codesandbox.io/s/affectionate-bouman-y201q?file=/src/index.tsx

After: https://codesandbox.io/s/bold-hoover-il7we?file=/src/react-reverse-portal.tsx

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

pimterry commented 3 years ago

I was surprised that this works, because nodeProps should never be falsey. It's initialized as {} (in getInitialPortalProps) and setPortalProps later on is only ever called with object values too.

I took a closer look at your example: the code there doesn't match this PR. Instead of !this.state.nodeProps, it uses this.state.nodeProps and so it never clones the child, which is why it works in the example case (but this disables using all props from OutPortals).

That said, the goal here 100% makes sense to me, and I'd love to avoid that unnecessary extra render if possible. How about doing this with something like Object.keys(nodeProps).length === 0 instead? I think that's what you're looking for.

It'd be helpful if you could copy your example in as a new storybook case too, so we can play with this within the project itself, against the committed code, rather than needing the sandboxes.

pimterry commented 3 years ago

Happy to continue this, but there's some changes required as above. I'll close it for now, but feel free to ping here if you're still interested in working on this.