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.

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

Set current fiber during before-mutation traversal #13219

Merged
merged 1 commit into from Jul 17, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 16, 2018

This sets current fiber during the getSnapshot traversal in the commit phase.

It's not currently observable because we don't have any warnings with stacks during that phase. But if we did, the stack would end up empty. With this change, we'd get the correct stack.

Verified this by changing a stack-less warning that occurs in this phase to have a stack (I didn't commit it though because I'm not sure we'd want a stack there).

The purpose of this PR isn't to fix anything, but to avoid future confusion in case we do add warnings with stacks during that phase. As far as I'm aware this was the only point in reconciliation where the stack is currently missing.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

LGTM overall, except the one thing noted.

const effectTag = nextEffect.effectTag;
if (__DEV__) {
ReactCurrentFiber.setCurrentFiber(nextEffect);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to put this inside of the while loop? I assume you meant to put it before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I meant to put it outside (just like we do in other effect traversal). It doesn't really give us anything to reset it on every iteration if we're going to set it again at the very beginning of every iteration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually sorry, I misunderstood your question (I thought you meant a different line).

But here, too, it was intentional, to match what we do in the other effect traversal. I put it inside the loop because I want to set the current fiber to the current effect as we iterate through effects. Putting it outside would defeat the purpose. Each effect is a different fiber.

Copy link
Contributor

Choose a reason for hiding this comment

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

D'oh yeah, I must have misread this previously.

@gaearon gaearon merged commit d6a0626 into facebook:master Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants