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

Don't fire the render phase update warning for class lifecycles #18330

Merged
merged 3 commits into from
Mar 18, 2020

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 17, 2020

Fixes #18178 (comment).

In 16.13, we added a new warning: Cannot update a component from inside a body of a function component. But as we see from reports like #18178 (comment), it fired for updates from inside of class lifecycles too. While arguably these are real issues (they happen in render phase) it's pretty noisy for legacy code.

In this PR, I do the following:

  • Change the wording (from inside a body of a function component -> while rendering). This more accurately describes when it would fire.
  • Change the deduping originally added in improve error message for cross-functional component updates #18316 to be more aggressive. In particular, I suggest to not fire more than one warning per callsite. This is because the actual target components tend to be disconnected and mostly unrelated due to abstractions like Redux. Let's focus on the part that fires the updates.
  • Limit the warning to function component bodies and class render lifecycles. Everything else will get muted. This is consistent with how our existing class "setState from another component's render" warning works.
    • As a possible follow-up, we could make both of these fire for other render-phase lifecycles like getDerivedStateFromProps, shouldComponentUpdate, and constructor. Maybe in strict mode only. However, we should keep them muted for componentWill* lifecycles including render phase ones.

Here is a table of the changes:

Screenshot 2020-03-17 at 21 10 07

Red means new behavior, blue means revert to 16.12 behavior.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 17, 2020
@gaearon gaearon requested a review from acdlite March 17, 2020 21:09
@codesandbox-ci
Copy link

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 933157f:

Sandbox Source
peaceful-mestorf-4ufj8 Configuration

@sizebot
Copy link

sizebot commented Mar 17, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 933157f

@sizebot
Copy link

sizebot commented Mar 17, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 933157f

@gaearon gaearon requested a review from bvaughn March 17, 2020 21:54
@gaearon gaearon merged commit fe1f79b into facebook:master Mar 18, 2020
@gaearon gaearon deleted the warn-less branch March 18, 2020 00:07
acdlite pushed a commit to acdlite/react that referenced this pull request Mar 19, 2020
…book#18330)

* Change the warning to not say "function body"

This warning is more generic and may happen with class components too.

* Dedupe by the rendering component

* Don't warn outside of render
acdlite pushed a commit to acdlite/react that referenced this pull request Mar 19, 2020
…book#18330)

* Change the warning to not say "function body"

This warning is more generic and may happen with class components too.

* Dedupe by the rendering component

* Don't warn outside of render
acdlite pushed a commit to acdlite/react that referenced this pull request Mar 19, 2020
…book#18330)

* Change the warning to not say "function body"

This warning is more generic and may happen with class components too.

* Dedupe by the rendering component

* Don't warn outside of render
@@ -1372,6 +1373,7 @@ function mountIndeterminateComponent(
context,
renderExpirationTime,
);
setIsRendering(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the component throws, setIsRendering won't be reset. This needs to be reset in the the work loop. Here:

// Reset module-level state that was set during the render phase.
resetContextDependencies();
resetHooksAfterThrow();
resetCurrentDebugFiberInDEV();

Copy link
Collaborator

@acdlite acdlite Mar 19, 2020

Choose a reason for hiding this comment

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

Never mind, gets reset by resetCurrentDebugFiberInDEV

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: too hard to fix "Cannot update a component from inside the function body of a different component."
5 participants