Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent stale legacy root from clearing a container (DRAFT) #18792

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Prevent stale legacy root from clearing a container (DRAFT) #18792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
350e02d
Don't clear a container because of a stale legacy root
Apr 30, 2020
b6312ff
Added test repro for FB error
Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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-
Copy link
Collaborator

@sebmarkbage sebmarkbage Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is off here because there's no concept of "discrete work" for legacy mode (and I'm removing it for CM too). At least there shouldn't be. Maybe for passive effects but that doesn't kick in here.

There's only Sync work in legacy mode.

What do you mean by discrete work here? It might that something is misnamed in a method or something.

It could also indicate a bigger bug in legacy mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebmarkbage This comment was based on Dominic's comment above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebmarkbage Upon calling scheduleUpdateOnFiber, you add an entry to rootsWithPendingDiscreteUpdates which is the problem in this issue. I'd assume your work of removing the concept of discrete work didn't include this particulat code-path?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't landed that part yet. Still blocked on some stuff.

Concretely I'm worried that we've accidentally landed "discrete" semantics in legacy mode at some point. I.e. some subtle breaking change.

// 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
View file
Open in desktop
Original file line number Diff line number Diff line change
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I opted to do this in the renderer in my PR was because this seemed host-specific. I see you reasoning for doing it earlier though, it avoids doing unecessary work before it's too late. Another approach could be to expose a host config function that does this check (as this is only related to the legacy roots, which doesn't necessarily affect other renderers) that can do this check for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess it is DOM renderer specific (then again, so is clearContainer itself really). I just thought it would be better to do this check earlier rather than in the perf-sensitive commit phase. Will be great to remove this check entirely eventually.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is DOM specific. Not sure what happens to this code in RN. I guess it would just schedule a Snapshot which is currently a noop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. RN doesn't have a way to clear its container anyway (since there's no native API).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this too in my previous comment. It's something we can address in a follow up.

Copy link
Contributor Author

@bvaughn bvaughn Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it is kind of renderer-specific, I don't know if I agree that moving it into the host config (and commit phase) is a good follow up change. Happy to talk more about it though. I agree that having a renderer-specific legacy check here isn't great either. Just felt less bad than incorrectly scheduling commit phase work.

Copy link
Contributor

@trueadm trueadm Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it sucks either way. I guess a complete phase legacy check is less bad as it has less overhead than my commit phase fix and we know we'll likely be removing it in the near future.

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
View file
Open in desktop
Original file line number Diff line number Diff line change
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