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

Replace passive effect context with boolean #18309

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Mar 14, 2020

Follow up to #18307

@bvaughn bvaughn requested a review from acdlite March 14, 2020 00:07
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 14, 2020
@@ -299,6 +298,10 @@ let spawnedWorkDuringRender: null | Array<ExpirationTime> = null;
// receive the same expiration time. Otherwise we get tearing.
let currentEventTime: ExpirationTime = NoWork;

// Dev only flag that tracks if passive effects are currently being flushed.
// We warn about state updates for unmounted components differently in this case.
let isFlushingPassiveEffects = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preference on the name? (I have none.)

@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 c2aafa7:

Sandbox Source
vigorous-dawn-8sqsr Configuration

@sizebot
Copy link

sizebot commented Mar 14, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against c2aafa7

@sizebot
Copy link

sizebot commented Mar 14, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against c2aafa7

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

Looks good to me, as does the naming.

@bvaughn bvaughn merged commit c0ed0a2 into facebook:master Mar 16, 2020
@bvaughn bvaughn deleted the change-state-update-warning-passive-effect-destroy-2 branch March 16, 2020 16:56
@@ -2448,6 +2454,10 @@ function flushPassiveEffectsImpl() {
nestedPassiveUpdateCount =
rootWithPendingPassiveEffects === null ? 0 : nestedPassiveUpdateCount + 1;

if (__DEV__) {
isFlushingPassiveEffects = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a guarantee code in between wouldn't throw even if user code throws (unless there's a bug in React)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User code is try/catched.

Though I wonder if I should have moved this before flushSyncCallbackQueue since that's where we reset prev exceution context.

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.

None yet

5 participants