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

Reset didReceiveUpdate in beginWork #16359

Merged
merged 1 commit into from Aug 12, 2019

Conversation

@sebmarkbage
Copy link
Member

commented Aug 11, 2019

This is a bad bug. It means that we sometimes inherit
didReceiveUpdate from a previous component's begin.

Effectively this only means that we're overrendering in some cases.

We should refactor to get rid of this as a global flag.

I don’t have a test case because it only showed up as an intermediate state in another test. Maybe we can come up with something simplified.

This is a bad bug. It means that we sometimes inherit
didReceiveUpdate from a previous component's begin.

Effectively this only means that we're overrendering in some cases.

We should refactor to get rid of this as a global flag.
@sizebot

This comment has been minimized.

Copy link

commented Aug 11, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@acdlite

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Chatted offline. This is a bug but it only affects the hooks bailout optimization, and only if all of the following are true:

  1. No new props
  2. There's a pending update on the fiber
  3. The update bails out during the render phase, which means it can't have bailed out due to the eager bailout optimization (the one that happens inside setState)
  4. The previous fiber that was processed also received an update.

So it's pretty rare. I wrote a test case. I'll submit separately.

didReceiveUpdate is also used by Suspense when hydrating, but I don't think it can affect that because of 3.

@acdlite acdlite merged commit 2716d91 into facebook:master Aug 12, 2019
12 checks passed
12 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: flow Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: lint_build Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test_build Your tests passed on CircleCI!
Details
ci/circleci: test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: test_source Your tests passed on CircleCI!
Details
ci/circleci: test_source_persistent Your tests passed on CircleCI!
Details
ci/circleci: test_source_prod Your tests passed on CircleCI!
Details
acdlite added a commit to acdlite/react that referenced this pull request Aug 12, 2019
@acdlite acdlite referenced this pull request Aug 12, 2019
@sebmarkbage

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

A test case that hits this for Suspense is added in #16369

acdlite added a commit that referenced this pull request Aug 13, 2019
This was referenced Oct 5, 2019
This was referenced Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.