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

Fix bug with double updates in a single batch #6650

Merged
merged 1 commit into from
Apr 29, 2016

Conversation

sophiebits
Copy link
Collaborator

The code here feels a little convoluted but I don't know how to make it clearer. Suggestions welcome.

cc @facebook/react-core

Fixes #2410. Fixes #6371. Fixes #6538.

I also manually tested the codepen in #3762 and verified it now works.

this.updateComponent(
transaction,
this._currentElement,
this._currentElement,
this._context,
this._context
);
} else {
this._updateBatchNumber = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to imply that enqueueUpdate is getting called on a component that does not need to be updated (meets neither of the two conditions above) - which seems bad/wrong. What am I missing here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. :( setState calls enqueueSetState and enqueueCallback which each create a separate batch. That is almost certainly unintended but I left it as-is for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is, enqueueCallback marks the component dirty which is harmless when done in conjunction with enqueueSetState but kinda silly when it can get split into a separate batch.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jimfb
Copy link
Contributor

jimfb commented Apr 29, 2016

Looks good to me. Let's ship it. 👍

@gaearon
Copy link
Collaborator

gaearon commented Apr 29, 2016

👍

Fixes facebook#2410. Fixes facebook#6371. Fixes facebook#6538.

I also manually tested the codepen in facebook#3762 and verified it now works.
@ghost
Copy link

ghost commented Apr 29, 2016

@spicyj updated the pull request.

@sophiebits sophiebits added this to the 15.0.x milestone Apr 29, 2016
@sebmarkbage
Copy link
Collaborator

As discussed in person: If componentWillUnmount is allowed to have side-effects (which I think it needs to because unsubscribing is a side-effect) which includes setState on another component, then it should be outside of the reconciliation pass which is supposed to be side-effect free. Maybe we should queue/defer unmounts?

@zpao
Copy link
Member

zpao commented May 10, 2016

Going to bump this to 15.x since it doesn't apply cleanly without some of the things we're taking there. Shouldn't matter too much in terms of timing.

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.

5 participants