Skip to content

Commit

Permalink
Prevent stale legacy root from clearing a container (DRAFT) (facebook…
Browse files Browse the repository at this point in the history
…#18792)

* Don't clear a container because of a stale legacy root

* Added test repro for FB error
  • Loading branch information
Brian Vaughn committed Apr 30, 2020
1 parent 5b89d35 commit ac533fd
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 4 deletions.
32 changes: 32 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Expand Up @@ -1280,4 +1280,36 @@ describe('ReactDOMFiber', () => {
);
expect(didCallOnChange).toBe(true);
});

it('unmounted legacy roots should never clear newer root content from a container', () => {
const ref = React.createRef();

function OldApp() {
const hideOnFocus = () => {
// This app unmounts itself inside of a focus event.
ReactDOM.unmountComponentAtNode(container);
};

return (
<button onFocus={hideOnFocus} ref={ref}>
old
</button>
);
}

function NewApp() {
return <button ref={ref}>new</button>;
}

ReactDOM.render(<OldApp />, container);
ref.current.focus();

ReactDOM.render(<NewApp />, container);

// Calling focus again will flush previously scheduled discerete work for the old root-
// but this should not clear out the newly mounted app.
ref.current.focus();

expect(container.textContent).toBe('new');
});
});
17 changes: 15 additions & 2 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Expand Up @@ -687,8 +687,21 @@ function completeWork(
// It's also safe to do for updates too, because current.child would only be null
// if the previous render was null (so the the container would already be empty).
//
// The additional root.hydrate check is required for hydration in legacy mode with no fallback.
workInProgress.effectTag |= Snapshot;
// The additional root.hydrate check above is required for hydration in legacy mode with no fallback.
//
// The root container check below also avoids a potential legacy mode problem
// where unmounting from a container then rendering into it again
// can sometimes cause the container to be cleared after the new render.
const containerInfo = fiberRoot.containerInfo;
const legacyRootContainer =
containerInfo == null ? null : containerInfo._reactRootContainer;
if (
legacyRootContainer == null ||
legacyRootContainer._internalRoot == null ||
legacyRootContainer._internalRoot === fiberRoot
) {
workInProgress.effectTag |= Snapshot;
}
}
}
updateHostContainer(workInProgress);
Expand Down
17 changes: 15 additions & 2 deletions packages/react-reconciler/src/ReactFiberCompleteWork.old.js
Expand Up @@ -685,8 +685,21 @@ function completeWork(
// It's also safe to do for updates too, because current.child would only be null
// if the previous render was null (so the the container would already be empty).
//
// The additional root.hydrate check is required for hydration in legacy mode with no fallback.
workInProgress.effectTag |= Snapshot;
// The additional root.hydrate check above is required for hydration in legacy mode with no fallback.
//
// The root container check below also avoids a potential legacy mode problem
// where unmounting from a container then rendering into it again
// can sometimes cause the container to be cleared after the new render.
const containerInfo = fiberRoot.containerInfo;
const legacyRootContainer =
containerInfo == null ? null : containerInfo._reactRootContainer;
if (
legacyRootContainer == null ||
legacyRootContainer._internalRoot == null ||
legacyRootContainer._internalRoot === fiberRoot
) {
workInProgress.effectTag |= Snapshot;
}
}
}
updateHostContainer(workInProgress);
Expand Down

0 comments on commit ac533fd

Please sign in to comment.