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.

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

Jump to bottom

prevProps in getDerivedStateFromProps() #40

Closed
wants to merge 2 commits into from
Closed

prevProps in getDerivedStateFromProps() #40

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
714ded0
`prevProps` in `getDerivedStateFromProps()`
catamphetamine Apr 5, 2018
76b8497
typo
catamphetamine Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
81 changes: 81 additions & 0 deletions text/text/0000-prevProps-in-getDerivedStateFromProps.md
View file
Open in desktop
@@ -0,0 +1,81 @@
- Start Date: (2018-04-05)
- RFC PR: (leave this empty)
- React Issue: (leave this empty)

# Summary

Not having `prevProps` in the new `getDerivedStateFromProps()` function is really inconvenient for a library I'm migrating because in `componentWillReceiveProps()` it compared a property to find out if it has changed, and only if it did then the component changed its own internal state.

# Basic example

I added `prevProps` to `getDerivedStateFromProps()` myself this way:

```js
constructor(props) {
super(props)

// Needs to store initial `this.props` here
// so that it's not `undefined` on the first
// `getDerivedStateFromProps()` call.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This example code wouldn't actually work. If "prev props" are the same as props during the initial render- the props.country !== state.props.country check below would never be true and the initial render would never have access to the derived state values until the first time the component was updated.

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn Actually, yeah, I should have wrote this.state = { props, derivedValue: ... } there instead of just this.state = { props }. I think if I push the correction now it will destroy comments so I guess I won't. If the constructor code is corrected to this.state = { props, derivedValue: ... }, and then if prevProps are implemented as the third argument of getDerivedStateFromProps(), then it will still make sense because developers will be able to drop props from this.state resulting in less cluttered state and avoiding the cases when a developer accidentally forgets to return props as part of new state in some of the many if/else branches of getDerivedStateFromProps().

Copy link

Choose a reason for hiding this comment

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

I agree with catamphetamine. The derivedValue could just have an initial value in the constructor. If the derivedValue issue is solved, could prevProps be provided using the value of props then?

this.state = { props }
}

// `state.props` is `prevProps`.
static getDerivedStateFromProps(props, state) {
if (props.country !== state.props.country) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In other words- if props and prevProps are the same for the initial render, this check won't work. That's why prevProps would need to be null or undefined the first time (if that parameter were to be provided).

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn

In other words- if props and prevProps are the same for the initial render, this check won't work.

If props and prevProps are the same for the initial render, this check won't "won't work". It will work. It just won't enter the if condition. Ain't nothing illegal in that.

That's why prevProps would need to be null or undefined the first time (if that parameter were to be provided).

Since your previous statement is falsy, this statement you're conducting from the previous one is also false.
prevProps would not need to be null or undefined the first time.
prevProps will simply be equal to this.props the first time.
And that's it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code, as written when I left this comment, would not work- because it would not set derivedValue anywhere for the initial render. That is all I was pointing out.

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn ok

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn I'm not mad at you, by the way. Just to make things clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries 🙂

Copy link

Choose a reason for hiding this comment

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

What about setting prevProps to {} on the first render?

Copy link
Author

Choose a reason for hiding this comment

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

@j-f1 I think it would break someone's code, because they could have

static propTypes = {
  value: PropTypes.shape({
    nestedValue: PropTypes.object
  }).isRequired
}

So they could expect props.value to always exist, and a developer could query props.value.nestedValue without any if/elses, so empty props would throw Cannot read "nestedValue" of undefined.

Copy link

Choose a reason for hiding this comment

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

@j-f1 This was already discussed here and in my opinion good reasons were given why this shouldn't happen.

Copy link

Choose a reason for hiding this comment

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

This part looks reasonable:

If props and prevProps are the same for the initial render, this check won't "won't work". It will work. It just won't enter the if condition. Ain't nothing illegal in that.

But I'm not sure what's the conclusion of this statement? If this statement is valid, could it be the reason to agree suing props as prevProps's first value?

return {
props,
derivedValue: ...
}
}
}
```

The whole `constructor()` thing is just for `prevProps` and feels bulky.
Copy link

Choose a reason for hiding this comment

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

This example seems a bit contrived, since you can write this all as:

state = {}

static getDerivedStateFromProps({ country }, state) {
  if (country !== state.country) {
    return { country, derivedValue ... }
  }
  return null
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we'll need some real-world examples to motivate this.

Copy link
Author

Choose a reason for hiding this comment

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

@jquense I can, but essentially this would be the same thing: storing part of previous props inside state - that's what you're basically suggesting. In my case I don't even need that country in my state - it's just the fact that it changed, nothing more. Consider the externally set default country, which can or can not change the currently selected internal state country depending on circumstances. Or consider localizedMessages: i don't need to store them in state, I just re-generate <select/> options based on those messages if they changed, and that's it.

The real-world example:

const new_default_country = this.props.country
const old_default_country = prevProps.country
const country = this.state.country

// If the default country changed.
// (e.g. in case of ajax GeoIP detection after page loaded)
// then select it but only if no phone number has been entered so far.
// Because if the user has already started inputting a phone number
// then he's okay with no country being selected at all ("International")
// and doesn't want to be disturbed, doesn't want his input to be screwed, etc.
if (new_default_country !== old_default_country && !country && !value) {
  return {
    ...,
    country : new_default_country
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

@gaearon CC ^^^^^


And I could be comparing more than just country - I'm also comparing `localeMessages` so that if they did change then I do

```js
// Imagine a user switched their locale, so the UI adapts in real-time.
if (prevProps.localeMessages !== props.localeMessages) {
this.setState({
selectOptions: generateLocalizedSelectOptions(props.localeMessages)
})
}
```

# Motivation

In my library `componentWillReceiveProps()` compared a property to find out if it has changed, and if it did then the component changed its own internal state. There's a suggestion to use `componentDidUpdate()` but it feels smelly because "derive state from props" is what my old `componentWillReceiveProps` really does so the name kinda fits in only if it did have `prevProps` argument.

# Detailed design

`getDerivedStateFromProps()` could take a third `prevProps` argument, with the first call simply being `getDerivedStateFromProps(this.props, this.state, this.props)` (inside constructor(), or whatever else it's now at). The first call would simply be a no-op.
Copy link
Collaborator

@bvaughn bvaughn Apr 11, 2018 •
edited

Choose a reason for hiding this comment

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

Regarding this proposal (getDerivedStateFromProps(this.props, this.state, this.props))- passing the same value as nextProps and prevProps for the initial call would be problematic for the same reasons as is shown by the code sample above:

Code sample from above

constructor(props) {
  super(props)

  // Needs to store initial `this.props` here
  // so that it's not `undefined` on the first
  // `getDerivedStateFromProps()` call.
  this.state = { props }
}

// `state.props` is `prevProps`.
static getDerivedStateFromProps(props, state) {
  if (props.country !== state.props.country) {
    return {
      props,
      derivedValue: ...
    }
  }
}

nextProps and prevProps (or nextProps and prevState.props) would always be the same for the initial call to getDerivedStateFromProps, and so the derivedValue key won't be computed for the first render.

If React were to pass a prevProps parameter to getDerivedStateFromProps, it would have to pick one of the following options for the initial call:

  1. Pass the initial props (which has the same downside as previously mentioned).
  2. Pass a null value (which would require additional null checks to be added before any prevProps references).
  3. Pass an empty object. (I see this as a variation of the previous option, as null checks would still be needed before any nested values.)

Passing a null initial value

If we were to pass a null initial value, the pattern for using this method would become something like:

static getDerivedStateFromProps(nextProps, prevState, prevProps) {
  const nextState = {};
  if (
    !prevProps ||
    prevProps.foo !== nextProps.foo
  ) {
    nextState.derivedFromFoo = derivedFromFoo;
  }
  if (
    !prevProps ||
    prevProps.bar !== nextProps.bar
  ) {
    nextState.derivedFromBar = derivedFromBar;
  }
  return nextState;
}

This does not look significantly different (or objectively better) to me than the current pattern:

static getDerivedStateFromProps(nextProps, prevState) {
  const nextState = {};
  if (nextProps.foo !== prevState.prevFoo) {
    nextState.prevFoo = nextProps.foo;
    nextState.derivedFromFoo = derivedFromFoo;
  }
  if (nextProps.bar !== prevState.prevBar) {
    nextState.prevBar = nextProps.bar;
    nextState.derivedFromBar = derivedFromBar;
  }
  return nextState;
}

Passing an empty object

Passing an empty object for the initial call would remove the need for certain types of null checks, but nested values would still need to check, e.g.:

if (
  prevProps.list != null &&
  prevProps.list.length !== nextProps.list.length
) {
  // ...
}

This would also make it more difficult to differentiate between values that are undefined because they were not passed by the user and values that are undefined because it's the initial render. (This distinction may be important in some cases.)

When is getDerivedStateFromProps necessary?

It's worth mentioning that getDerivedStateFromProps is not a lifecycle that's commonly needed. If state is not expensive to derive, it can just be done in render. Even if it is expensive- there are many small memoization packages on NPM that could be used in order to just derive values in render. For example:

import memoize from "lodash.memoize";

class Example {
  getDerivedValue = memoize(value => {
    // ...
  });

  render() {
    const derivedFromFoo = this.getDerivedValue(this.props.foo);
    const derivedFromBar = this.getDerivedValue(this.props.bar);

    // Render with this.props, this.state, and derived values ...
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

@bvaughn wow, that's a really elaborate comment.

nextProps and prevProps (or nextProps and prevState.props) would always be the same for the initial call to getDerivedStateFromProps, and so the derivedValue key won't be computed for the first render.

Yeah, you already mentioned that in your initial review, and the two of us already discussed it above, and came to a conclusion that it's a typo that I didn't want to push to not destroy the comments you left. In reality it will be this.state = { derivedProp: ..., props } so it won't matter that the first time the if block won't be stepped into - the constructor takes care of that edge case.

It's worth mentioning that getDerivedStateFromProps is not a lifecycle that's commonly needed.

true

If state is not expensive to derive, it can just be done in render.

true true

Even if it is expensive- there are many small memoization packages on NPM that could be used in order to just derive values in render.

yes again

and this brings us to the concluding question: "When is getDerivedStateFromProps() actually needed?".

As far as I saw you tried to answer that question through your little twitter survey and I guess the answer is: "When developers for whatever reasons resort to doing some crazy-ass borderline barely-legal stuff", and that almost always involves comparing this.props and prevProps as far as I'm telling.

So, this brings us to the final conclusion that prevProps argument is still needed for the new getDerivedStateFromProps() lifecycle method. Because developers only resort to getDerivedStateFromProps() when they really need something like prevProps, otherwise they'd get away with just a stateless component + "memoization".

That's my current idea of this whole thing.

Copy link
Collaborator

@bvaughn bvaughn Apr 11, 2018 •
edited

Choose a reason for hiding this comment

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

Yeah, you already mentioned that in your initial review, and the two of us already discussed it above, and came to a conclusion that it's a typo

Right. I mentioned it again in this response not to point out the typo- but because I think it's a mistake that might be pretty common. (It's easy to overlook that case.) Sorry if it seemed like I was picking on the example specifically. That wasn't my intent!

The Twitter survey was only tangentially related to this RFC. 😄 Ryan and I were discussing what people actually use cWRP/gDSFP for, and how many of those cases might be better handled by memoization, etc. and it made me curious to learn more about how others were using it.

Copy link

@wood1986 wood1986 Apr 11, 2018 •
edited

Choose a reason for hiding this comment

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

Seriously?! @bvaughn I saw your tweet but I thought you were talking other stuff.

Anyway, is it possible to let us define the prevProps outside the getDerivedStateFromProps? If so, it will ease your concerns.

Something is similar to propTypes.

import PropTypes from 'prop-types';

class Greeting extends React.Component {
  render() {
    return (
      <h1>Hello, {this.props.name}</h1>
    );
  }
}

Greeting.propTypes = {
  name: PropTypes.string
};

Greeting.prevProps = {
  // Hey devs, it is your responsibility
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seriously?! @bvaughn I saw your tweet but I thought you were talking other stuff.

To be clear, the Tweet wasn't related to this RFC. I was just curious about what sort of things people are using componentWillReceiveProps or getDerivedStateFromProps for 😄

You could define a custom static property on your component, yes, but then you would need to update it any time props changed which seems like more work than using one of the patterns suggested above?

Copy link

Choose a reason for hiding this comment

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

You could define a custom static property on your component, yes, but then you would need to update it any time props changed which seems like more work than using one of the patterns suggested above?

I would expect this is read-only, one-time-use and a reserved react property. It is just for the first call to getDerivedStateFromProps. Then you do not have to care null, {} or same props for the initial prevProps value. After the first call to getDerivedStateFromProps, React will delete it. Yes, the downside is a new rule is introduced and if devs do not specified it, it still have the same issue. Forget and close it.

Copy link

Choose a reason for hiding this comment

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

I've read the above but am not very sure why this:

Pass the initial props (which has the same downside as previously mentioned).

I think I've only found that some state values could not be derived if the first props.x === prevProps.x fails, but it can be solved by providing a default state value in the constructor.


This behaviour complies with the official docs [explicitly state](https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops):

> Note that if a parent component causes your component to re-render, this method will be called even if props have not changed. You may want to compare new and previous values if you only want to handle changes.

I.e. it says that the new props aren't neccessarily different from the old ones, so doing `getDerivedStateFromProps(this.props, this.state, this.props)` wouldn't be illegal in any way and wouldn't contradict the already cemented behaviour for this new function.

# Drawbacks

None I can think of.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is at least one major drawback: This proposed change would break any applications that:

  1. Use react 16.3.0 or 16.3.1 and...
  2. Use an open source component polyfilled by react-lifecycles-compat

In the above case, React would call getDerivedStateFromProps with only 2 parameters- even if the library component depends on the prevProps parameter.

To be clear, this drawback is not technically inherent to this proposal. (It exists because we've already released the API.) So it's not the focus of my response, but I think it is a consideration worth mentioning.


# Alternatives

I guess this would work but it doesn't feel semantically right given that there already is `getDerivedStateFromProps()`.

```js
componentDidUpdate(prevProps) {
if (this.props.country !== prevProps.country) {
this.setState({
derivedValue: ...
})
}
}
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using componentDidUpdate, as shown above, would result in additional, unnecessary renders.

The React docs show an alternative pattern, which is to store a copy of the props you want to compare (in this case country) in state:

static getDerivedStateFromProps(nextProps, prevState) {
  if (prevState.prevCountry !== nextProps.country) {
    return {
      prevCountry: nextProps.country,
      derivedValue: yourValueHere
    };
  }
}


# Adoption strategy

If the third argument is added there's no migration strategy. It could be the first one, but React 16.3 is already released.