Skip to content

async-safe, static lifecycle hooks #6

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.

Sign up for GitHub

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

Merged
acdlite merged 22 commits into from
Jan 19, 2018
Merged

async-safe, static lifecycle hooks #6

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9818f57
Initial proposal for async-friendly, static lifecycle hooks
bvaughn Dec 8, 2017
839547f
Added a note about the prefix
bvaughn Dec 8, 2017
b7189cc
Made deprecation/rename clearer
bvaughn Dec 8, 2017
aad6845
Hopefully clarified the computeMemoizeData() placeholder example
bvaughn Dec 8, 2017
99988da
Renamed example method for improved clarity
bvaughn Dec 9, 2017
4a34d25
Added note about aEL being unsafe in cWM
bvaughn Dec 9, 2017
902d4b3
Fixed typo
bvaughn Dec 9, 2017
3df7ce6
Fixing typo
bvaughn Dec 9, 2017
1864c93
Removed Facebook-specific terminology
bvaughn Dec 9, 2017
428758b
Tweaked wording for clarity
bvaughn Dec 9, 2017
cfcb7e8
Reorganization (part 1, more to come)
bvaughn Dec 9, 2017
536084d
Added some more focused examples
bvaughn Dec 9, 2017
a1431a4
Added a comment about calling setState on a possibly unmounted component
bvaughn Dec 9, 2017
3c6132e
Cancel async request on unmount in example
bvaughn Dec 9, 2017
4425dbe
Tweaking examples based on Dan's feedback
bvaughn Dec 9, 2017
67272ce
Renamed deriveStateFromProps to getDerivedStateFromNextProps
bvaughn Dec 13, 2017
c7f6728
Renamed prefetch to optimisticallyPrepareToRender
bvaughn Dec 13, 2017
bb2d246
Added `render` to a list
bvaughn Dec 14, 2017
8618f70
Removed static optimisticallyPrepareToRender() in favor of render()
bvaughn Dec 19, 2017
8f6c20e
Renamed getDerivedStateFromNextProps to getDerivedStateFromProps
bvaughn Jan 18, 2018
e05e317
Updated when getDerivedStateFromProps is called and what its argument…
bvaughn Jan 18, 2018
7042a2a
Renamed unsafe_* to UNSAFE_*
bvaughn Jan 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Renamed deriveStateFromProps to getDerivedStateFromNextProps
  • Loading branch information
@bvaughn
bvaughn committed Dec 13, 2017
commit 67272ce6a9c6cc93610d7775f25a9d8d86f4b83c
14 changes: 7 additions & 7 deletions text/0000-static-lifecycle-methods.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ At a high-level, I propose the following additions/changes to the component API.

```js
class ExampleComponent extends React.Component {
static deriveStateFromProps(props, state, prevProps) {
static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
Copy link
Contributor

@TrySound TrySound Dec 15, 2017

Choose a reason for hiding this comment

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

What about simple

class Example extends React.Component {
  static deriveState(nextProps, prevProps, prevState) {
    return null;
  }
  // or
  static getDerivedState(nextProps, prevProps, prevState) {
    return null;
  }
}

I think it's pretty neat. Keeping static user methods doesn't make sense. So why don't keep these names simple and fast for write?

Copy link
Collaborator Author

@bvaughn bvaughn Dec 19, 2017

Choose a reason for hiding this comment

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

I don't feel strongly about the name getDerivedState vs getDerivedStateFromNextProps. If others do, they're welcome to weigh in.

// Called before a mounted component receives new props.
// Return an object to update state in response to prop changes.
// Return null to indicate no change to state.
Expand Down Expand Up @@ -172,10 +172,10 @@ class ExampleComponent extends React.Component {
derivedData: computeDerivedState(this.props)
};

static deriveStateFromProps(props, state, prevProps) {
if (props.someValue !== prevProps.someValue) {
static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
if (nextProps.someValue !== prevProps.someValue) {
return {
derivedData: computeDerivedState(props)
derivedData: computeDerivedState(nextProps)
};
}

Expand Down Expand Up @@ -375,11 +375,11 @@ The purpose of this method is to initiate asynchronous request(s) as early as po

Avoid introducing any non-idempotent side-effects, mutations, or subscriptions in this method. For those use cases, use `componentDidMount`/`componentDidUpdate` instead.

### `static deriveStateFromProps(props: Props, state: State, prevProps: Props): PartialState | null`
### `static getDerivedStateFromNextProps(nextProps: Props, prevProps: Props, prevState: Props): PartialState | null`
Copy link
Member

@koba04 koba04 Jan 17, 2018

Choose a reason for hiding this comment

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

Missing prevState in arguments?

Copy link

@jamesplease jamesplease Jan 17, 2018

Choose a reason for hiding this comment

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

Is it not the third argument?

Copy link
Member

@koba04 koba04 Jan 17, 2018

Choose a reason for hiding this comment

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

Ah, right. I'm confused by the type(prevState: Props).
prevState: State is better.

Copy link
Collaborator Author

@bvaughn bvaughn Jan 17, 2018

Choose a reason for hiding this comment

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

Oh, whoops. That looks like a Flow-type typo.


This method is invoked before a mounted component receives new props. Return an object to update state in response to prop changes. Return null to indicate no change to state.
Copy link
Member

@koba04 koba04 Jan 17, 2018

Choose a reason for hiding this comment

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

Return null to indicate no change to state

What's the benefit of this API design?
What about undefined something like this?

static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
  if (someCondition) {
    return;
  }
}

// or
static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
}

I prefer to return prevState explicitly if we don't need to update the state.
Return null to indicate no change to state might be unclear for developers.

Also, waring for returning null or undefined is good for me.

static getDerivedStateFromNextProps(nextProps, prevProps, prevState) {
  if (!someCondition) {
  }
  return prevState;
}

Copy link
Collaborator Author

@bvaughn bvaughn Jan 17, 2018

Choose a reason for hiding this comment

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

Return null to indicate no change to state

What's the benefit of this API design?

It signals a clear intent. Undefined return value may indicate someone forget a default case or an else statement, etc

Copy link
Member

@koba04 koba04 Jan 17, 2018

Choose a reason for hiding this comment

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

@bvaughn Thank you for your response!

What happened if getDerivedStateFromNextProps returns undefined? It is ignored and the state won't be changed?

It signals a clear intent.

In that case, Return prevState seems to be clearer than return null.
Why is null treated as a special case?

Copy link
Collaborator Author

@bvaughn bvaughn Jan 17, 2018

Choose a reason for hiding this comment

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

No problem. 😄

What happened if getDerivedStateFromNextProps returns undefined? It is ignored and the state won't be changed?

Probably log a warning, suggesting the user return null instead, to avoid the potential accidental omission of a return statement like I mentioned before.

In that case, Return prevState seems to be clearer than return null.
Why is null treated as a special case?

I think null is safer. We could compare the identity of the returned value to prevState, but what if a user modified values on prevState (same object, but mutated keys - although to be clear, this should not be done).

Copy link
Member

@koba04 koba04 Jan 17, 2018

Choose a reason for hiding this comment

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

Thanks!

Probably log a warning, suggesting the user return null instead, to avoid the potential accidental omission of a return statement like I mentioned before.

👍

I think null is safer. We could compare the identity of the returned value to prevState, but what if a user modified values on prevState (same object, but mutated keys - although to be clear, this should not be done).

I got it.
But why is comparing the identity of the returned value to prevState needed?
Will the result be used for something?
Are there any problems to use a value returned from getDerivedStateFromNextProps without the comparing?

const nextState = type.getDerivedStateFromNextProps(nextProps, prevProps, prevState);
if (/* validate nextState */) {
}
filter.memoizeState = nextState;

Copy link
Collaborator Author

@bvaughn bvaughn Jan 17, 2018

Choose a reason for hiding this comment

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

But why is comparing the identity of the returned value to prevState needed?
Will the result be used for something?

Because the returned value, like the value passed to setState, can be a partial state- meaning that we need to (eventually) merge it into the current state. Merging the current state with itself is doing work unnecessarily- and a subsequent call to render() (the default behavior when state is updated) is more work- so it's nice for us to have a faster way to determine there's no actual work to be done.

Copy link
Member

@koba04 koba04 Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks That makes sense!I missed the return type is PartialState | null.
setState(updater) can accept undefined to indicate no change not only null so it might also make sense to accept undefined as well as a perspective of API consistency.

Copy link
Collaborator Author

@bvaughn bvaughn Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That's not an unreasonable suggestion, but I disagree with it. To me, it seems more likely that one might forget to handle a return case than it is for one to accidentally call this.setState with an undefined value. React's DEV warnings are there to protect users from accidental oversights, and this (in my mind) is more similar to forgetting to return a value from render (which we also don't support).


Note that React may call this method even if the props have not changed. If calculating derived data is expensive, compare new and previous prop values to conditionally handle changes.
Note that React may call this method even if the props have not changed. If calculating derived data is expensive, compare next and previous props to conditionally handle changes.
Copy link

@amannn amannn Jan 22, 2018

Choose a reason for hiding this comment

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

@bvaughn The PR for React included
a note that getDerivedStateFromProps doesn't receive previous props. Should this sentence be updated?

Copy link
Collaborator Author

@bvaughn bvaughn Jan 22, 2018

Choose a reason for hiding this comment

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

The wording here is a bit ambiguous but I think that's okay. Current and previous props can be compared, as shown in this example

Copy link

@amannn amannn Jan 22, 2018

Choose a reason for hiding this comment

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

Ah, previous props in this case means props that have been synced to the local state and can be access via prevState, right? Alright then, I thought this was left over since I think the prevProps parameter got removed in a later iteration.

Copy link

@myitcv myitcv Jan 22, 2018

Choose a reason for hiding this comment

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

So @bvaughn does this mean that, given our previous conversation, I have to "sync" all my props to state in order to compare previous props with the new props? Feels like I'm missing something here.

Copy link
Collaborator Author

@bvaughn bvaughn Jan 22, 2018

Choose a reason for hiding this comment

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

Not all props, only ones that are directly used to derive state, and only if it's an expensive computation.

Copy link
Member

@gaearon gaearon Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Does storing "mirrored" props in state in addition to their derivatives not go against the React principle of state as a minimal representation?

It's a bit different though. You're not storing a "mirrored" version, you're storing the previous version. Which is a special case of an accumulation (that happens to completely discard the current value). Accumulation is exactly the purpose of getDerivedStateFromProps and since you already added it (presumably for accumulating something), adding another field to it doesn't hurt.

I guess one could say that a "mirrored" version would also be a special case of accumulation (that discards the previous value) but that's sophism IMO :-)

Copy link

@billyjanitsch billyjanitsch Jan 26, 2018

Choose a reason for hiding this comment

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

since you already added it (presumably for accumulating something), adding another field to it doesn't hurt

Yeah, I see what you mean. I just feel that it muddies the message of "don't put unnecessary stuff in state" where "necessary" no longer precisely means "used in render". Personally I find this a bigger / less intuitive exception to a rule than prevProps's nullability (the reason for which is pretty obvious -- no prevProps on first render). I suspect newer devs would struggle more with the former.

Anyway I see the trade-off, and I appreciate your willingness to explain! 😊

Copy link
Member

@gaearon gaearon Jan 28, 2018

Choose a reason for hiding this comment

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

I just feel that it muddies the message of "don't put unnecessary stuff in state" where "necessary" no longer precisely means "used in render".

We might actually revisit this one ^^

With async React it might not be safe to put certain things on the instance fields. See facebook/react#10580.

Copy link

@myitcv myitcv Jan 29, 2018

Choose a reason for hiding this comment

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

@bvaughn @gaearon I've tried to flesh out what I mean using a Typescript example - see this gist - to compare the two approaches.

I've extracted this example from a real life component that has more props, but hopefully you get the idea: any component would have to follow a similar pattern.

There are three files in the gist:

  1. the component written as per this proposal
  2. the component written assuming a signature of static getDerivedStateFromProps(nextProps: Props, prevProps: Props, initial: boolean): State
  3. the diff between the two

Notice file 2 is using a slightly different signature to the proposed in my last response; I agree with @gaearon the nullability issue is best avoided, hence prevProps is not nullable and an explicit boolean is provided to indicate whether it's the initial derivation or not.

Does the diff help to show what I mean about the boilerplate required when we have to "sync" to state?

Thanks

Copy link
Collaborator Author

@bvaughn bvaughn Jan 29, 2018

Choose a reason for hiding this comment

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

Notice file 2 is using a slightly different signature to the proposed in my last response; I agree with @gaearon the nullability issue is best avoided, hence prevProps is not nullable and an explicit boolean is provided to indicate whether it's the initial derivation or not.

Previous props would still need to be null or undefined in this case, no? Or even more confusingly, they would equal current props.

Anyway~ thanks for the feedback, Paul. I understand and appreciate your concern about verbosity. 🙇

This proposal was discussed in length in December, accepted, and merged into the React repo a couple of weeks ago. I'm going to lock down the thread at this point because I think we've already committed to this API.

Hope you understand!


React does not call this method before the intial render/mount and so it is not called during server rendering.
Copy link
Member

@jlongster jlongster Dec 9, 2017

Choose a reason for hiding this comment

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

It would be ideal IMHO if we could somehow coalesce this use case for initial mount and updating props. It's common for me to pull out a function like this already so that I can call it both during mount and componentWillReceiveProps, because you often need to do the same derivation during mount. I'd argue that's the more common case.

I haven't read through this in its entirety though, maybe that's addressed elsewhere.

There are also some cases (much rarer) where I use an instance variable during this derivation process. So it'd be unfortunate not to have that there, but with async maybe there are pitfalls that I'm not aware of (I should read all of this through).

Copy link
Member

@gaearon gaearon Dec 9, 2017

Choose a reason for hiding this comment

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

I agree I’d like to have this called on initial mount too. Curious to learn more why this was decided against here.

Copy link

@greggb greggb Dec 9, 2017

Choose a reason for hiding this comment

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

As proposed, the name for this method isn't clear about if it will render on mount or not. The componentWillReceiveProps hook seems clear because a component will always initially render with props, so it wouldn't need a life cycle hook. I foresee some confusion with the current name.

Having it run on initial render would be a great feature add. Just today a dev on my team asked why we end up having so much similar logic in ComponentDidMount and ComponentReceiveProps.

Copy link
Member

@gaearon gaearon Dec 9, 2017

Choose a reason for hiding this comment

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

The componentWillReceiveProps hook seems clear because a component will always initially render with props, so it wouldn't need a life cycle hook. I foresee some confusion with the current name.

It’s clear in retrospect because you already know it. In practice I see engineers expect it to fire on initial mount very often.

Copy link

@greggb greggb Dec 9, 2017

Choose a reason for hiding this comment

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

That's something I wouldn't have guessed. Thanks for the insight

Copy link
Collaborator Author

@bvaughn bvaughn Dec 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Setting an instance property (state) in a constructor isn't a side effect? Isn't that the only thing a constructor is meant for? Maybe I misunderstanding what you mean?

Copy link
Member

@trueadm trueadm Dec 9, 2017

Choose a reason for hiding this comment

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

I frequently see things in the constructor being side-effectful, like Promises, event emitters, accessing globals etc.

Copy link
Collaborator Author

@bvaughn bvaughn Dec 10, 2017

Choose a reason for hiding this comment

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

Sure, but initializing state via the constructor (or just as a class property, which is nicer still IMO) isn't a side effect. Again, maybe I'm misunderstanding what you're saying.

Are you suggesting that we shouldn't recommend using the constructor for anything (even state) because people might use it for other things? If so, I don't think I agree.

Copy link
Member

@trueadm trueadm Dec 10, 2017

Choose a reason for hiding this comment

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

I’m suggesting we make the initilization of state pure by returning from a function given some props passed in as arguments. The constructor still needs to exist for side-effectual, non-pure operations such as binding methods of the class, creating class methods as instanve variables and calling super(). It would also really help the compilation effort, as we can evaluate pure functions ahead of time given it has to return the initial state.

Copy link
Collaborator Author

@bvaughn bvaughn Dec 11, 2017

Choose a reason for hiding this comment

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

I don't understand the benefit of removing state initialization from the constructor if other initialization si done there. How does this help compilation?


Expand All @@ -395,7 +395,7 @@ This method will log a deprecation warning in development mode recommending that

### `componentWillReceiveProps` -> `unsafe_componentWillReceiveProps`

This method will log a deprecation warning in development mode recommending that users either rename to `unsafe_componentWillReceiveProps` or use the new static `deriveStateFromProps` method instead. It will be removed entirely in version 17.
This method will log a deprecation warning in development mode recommending that users either rename to `unsafe_componentWillReceiveProps` or use the new static `getDerivedStateFromNextProps` method instead. It will be removed entirely in version 17.

# Drawbacks

Expand Down