-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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.
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
Prevent stale legacy root from clearing a container (DRAFT) #18792
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b6312ff:
|
Details of bundled changes.Comparing: 53d68b3...b6312ff react-test-renderer
react-dom
react-art
react-native-renderer
react-reconciler
Size changes (experimental) |
Details of bundled changes.Comparing: 53d68b3...b6312ff react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
Size changes (stable) |
The thing I'm not able to repro in a test yet (but that we're seeing within Facebook) is
|
@bvaughn Nice findings. It might have something to do with the fact that when we call |
I think I got it! |
Thanks @trueadm, that comment was very helpful in helping me realize the thing I was missing in my test. |
e486c16
to
6366862
Compare
6366862
to
b6312ff
Compare
This is awesome! I wonder if we use |
No, I don't think so. This only applies to legacy roots. Also if you tried to create a new root ( |
// can sometimes cause the container to be cleared after the new render. | ||
const containerInfo = fiberRoot.containerInfo; | ||
const legacyRootContainer = | ||
containerInfo == null ? null : containerInfo._reactRootContainer; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more than happy with this change - awesome stuff! My comment can be worked on in the future as a TODO, as I don't see it blocking this.
Thanks for the review, Dominic. I'm going to merge this to hopefully unblock you on the sync. (Let me know if you need someone to take over the sync btw.) I can follow up later if there are concerns or suggested changes from others. |
|
||
ReactDOM.render(<NewApp />, container); | ||
|
||
// Calling focus again will flush previously scheduled discerete work for the old root- |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
// can sometimes cause the container to be cleared after the new render. | ||
const containerInfo = fiberRoot.containerInfo; | ||
const legacyRootContainer = | ||
containerInfo == null ? null : containerInfo._reactRootContainer; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes the legacy root bug we observed after the recent sync (caused by #18730).
Alternate fix to #18781.