Skip to content

Commit cf0f714

Browse files
committed
More reliably handle race conditions in mount/unmount
The portal now directly owns its own mounting into the DOM, so can guarantee that it's in the right place, and always tidied up when its moved into a new place. In addition, we now handle various funky & racey cases, including updating when duplicate OutPortals are removed, and overlapping portal mountings.
1 parent fdbae73 commit cf0f714

File tree

2 files changed

+98
-13
lines changed

2 files changed

+98
-13
lines changed

src/index.tsx

Lines changed: 56 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,12 @@ export interface PortalNode<C extends Component<any> = Component<any>> extends H
1111
setPortalProps(p: ComponentProps<C>): void;
1212
// Used to track props set before the InPortal hooks setPortalProps
1313
getInitialPortalProps(): ComponentProps<C>;
14+
// Move the node from wherever it is, to this parent, replacing the placeholder
15+
mount(newParent: Node, placeholder: Node): void;
16+
// If mounted, unmount the node and put the initial placeholder back
17+
// If an expected placeholder is provided, only unmount if that's still that was the
18+
// latest placeholder we replaced. This avoids some race conditions.
19+
unmount(expectedPlaceholder?: Node): void;
1420
}
1521

1622
interface InPortalProps {
@@ -20,14 +26,52 @@ interface InPortalProps {
2026

2127
export const createPortalNode = <C extends Component<any>>(): PortalNode<C> => {
2228
let initialProps = {} as ComponentProps<C>;
23-
return Object.assign(document.createElement('div'), {
29+
30+
let parent: Node | undefined;
31+
let lastPlaceholder: Node | undefined;
32+
33+
const portalNode = Object.assign(document.createElement('div'), {
2434
setPortalProps: (props: ComponentProps<C>) => {
2535
initialProps = props;
2636
},
2737
getInitialPortalProps: () => {
2838
return initialProps;
39+
},
40+
mount: (newParent: Node, newPlaceholder: Node) => {
41+
if (newPlaceholder === lastPlaceholder) {
42+
// Already mounted - noop.
43+
return;
44+
}
45+
portalNode.unmount();
46+
47+
newParent.replaceChild(
48+
portalNode,
49+
newPlaceholder
50+
);
51+
52+
parent = newParent;
53+
lastPlaceholder = newPlaceholder;
54+
},
55+
unmount: (expectedPlaceholder?: Node) => {
56+
if (expectedPlaceholder && expectedPlaceholder !== lastPlaceholder) {
57+
// Skip unmounts for placeholders that aren't currently mounted
58+
// They will have been automatically unmounted already by a subsequent mount()
59+
return;
60+
}
61+
62+
if (parent && lastPlaceholder) {
63+
parent.replaceChild(
64+
lastPlaceholder,
65+
portalNode
66+
);
67+
68+
parent = undefined;
69+
lastPlaceholder = undefined;
70+
}
2971
}
3072
});
73+
74+
return portalNode;
3175
};
3276

3377
export class InPortal extends React.PureComponent<InPortalProps, { nodeProps: {} }> {
@@ -88,28 +132,27 @@ export class OutPortal<C extends Component<any>> extends React.PureComponent<Out
88132
}
89133

90134
componentDidMount() {
135+
const node = this.props.node as PortalNode<C>;
136+
91137
const placeholder = this.placeholderNode.current!;
92-
placeholder.parentNode!.replaceChild(
93-
this.props.node,
94-
placeholder
95-
);
138+
const parent = placeholder.parentNode!;
139+
node.mount(parent, placeholder);
96140
this.passPropsThroughPortal();
97141
}
98142

99143
componentDidUpdate() {
144+
// We re-mount on update, just in case we were unmounted (e.g. by
145+
// a second OutPortal, which has now been removed)
146+
const node = this.props.node as PortalNode<C>;
147+
const placeholder = this.placeholderNode.current!;
148+
const parent = placeholder.parentNode!;
149+
node.mount(parent, placeholder);
100150
this.passPropsThroughPortal();
101151
}
102152

103153
componentWillUnmount() {
104154
const node = this.props.node as PortalNode<C>;
105-
106-
if (node.parentNode) {
107-
node.parentNode.replaceChild(
108-
this.placeholderNode.current!,
109-
node
110-
);
111-
this.props.node.setPortalProps({});
112-
}
155+
node.unmount(this.placeholderNode.current!);
113156
}
114157

115158
render() {

stories/index.stories.js

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,48 @@ storiesOf('Portals', module)
145145
</div>
146146
});
147147
})
148+
.add('renders reliably, even with frequent changes and multiple portals', () => {
149+
const portalNode = portals.createPortalNode('div');
150+
151+
const Target = (p) => p.value.toString();
152+
153+
const Parent = () => {
154+
const [state, setState] = React.useState(false);
155+
156+
// Change frequently, to hunt for race conditions. On leaving this story, this might
157+
// complain about calling setState after unmount - that's totally fine, we don't care.
158+
// There should be no other errors though.
159+
setTimeout(() => {
160+
setState(!state);
161+
}, 100);
162+
163+
return <div>
164+
Portal flickers between 2 / 3 / nothing here:
165+
{ state
166+
// What happens if you render the same portal twice?
167+
? <>
168+
<portals.OutPortal node={portalNode} value={1} />
169+
<portals.OutPortal node={portalNode} value={2} />
170+
</>
171+
// What happens if you switch from 2 portals to 1, to 2 to zero, at random?
172+
: Math.random() > 0.5
173+
? <portals.OutPortal node={portalNode} value={3} />
174+
: null
175+
}
176+
</div>;
177+
}
178+
179+
return <div>
180+
<div>
181+
Portal defined here:
182+
<portals.InPortal node={portalNode}>
183+
<Target value='unmounted' />
184+
</portals.InPortal>
185+
</div>
186+
187+
<Parent />
188+
</div>;
189+
})
148190
.add('Example from README', () => {
149191
const MyExpensiveComponent = () => 'expensive!';
150192

0 commit comments

Comments
 (0)