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 instance vars before calling commit phase lifecycles #10481

Merged
merged 1 commit into from Aug 17, 2017

Conversation

Projects
None yet
3 participants
@acdlite
Member

acdlite commented Aug 17, 2017

Fixes an issue where if a component throws, its instance variables are not reset before componentWillUnmount is called. See https://jsfiddle.net/m3pL3yaj/ for an illustration.

To protect against issues like this, we should always set this.props and this.state right before calling a lifecycle method, as if they were explicit arguments.

@acdlite acdlite requested a review from sebmarkbage Aug 17, 2017

@gaearon gaearon referenced this pull request Aug 17, 2017

Closed

React 16 Umbrella ☂️ (and 15.5) #8854

117 of 120 tasks complete
@sebmarkbage

arguments, slice and apply are all things that are often slow paths. can we do less meta programming and more programming?

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 17, 2017

Member

@sebmarkbage Updated, now with more programming

Member

acdlite commented Aug 17, 2017

@sebmarkbage Updated, now with more programming

@@ -495,6 +499,8 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
if (__DEV__) {
startPhaseTimer(finishedWork, 'componentDidMount');
}
instance.props = finishedWork.memoizedProps;
instance.state = finishedWork.memoizedState;

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 17, 2017

Member

Is there any observable way these wouldn't already be the same?

Other than the user setting them themselves? (I suppose if they do, then this is a breaking change now.)

@sebmarkbage

sebmarkbage Aug 17, 2017

Member

Is there any observable way these wouldn't already be the same?

Other than the user setting them themselves? (I suppose if they do, then this is a breaking change now.)

This comment has been minimized.

@acdlite

acdlite Aug 17, 2017

Member

Not that I can think of, more concerned about the case I can't think of, like how I didn't consider error boundaries. Is this a perf concern?

@acdlite

acdlite Aug 17, 2017

Member

Not that I can think of, more concerned about the case I can't think of, like how I didn't consider error boundaries. Is this a perf concern?

This comment has been minimized.

@acdlite

acdlite Aug 17, 2017

Member

I can think of some in async mode, just not sync mode.

@acdlite

acdlite Aug 17, 2017

Member

I can think of some in async mode, just not sync mode.

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 17, 2017

Member

What about async mode? I can't even think of them there.

@sebmarkbage

sebmarkbage Aug 17, 2017

Member

What about async mode? I can't even think of them there.

@@ -505,6 +511,8 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
if (__DEV__) {
startPhaseTimer(finishedWork, 'componentDidUpdate');
}
instance.props = finishedWork.memoizedProps;
instance.state = finishedWork.memoizedState;

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 17, 2017

Member

Same for didUpdate.

@sebmarkbage

sebmarkbage Aug 17, 2017

Member

Same for didUpdate.

@acdlite acdlite merged commit caaa0b0 into facebook:master Aug 17, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@bvaughn bvaughn referenced this pull request Sep 7, 2017

Closed

React 16 RC #10294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment