React: Add oldProps as additional argument to getDerivedStateFromProps ?

[jharris4]

Do you want to request a feature or report a bug?

feature

What is the current behavior?

getDerivedStateFromProps only receives the nextProps and previousState as arguments.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

The deprecated componentWillReceiveProps(nextProps) used to allow code like this.props.foo !== nextProps.foo. With the new getDerivedStateFromProps function, there's no choice (because it is a static method) but to constantly copy nextProps.foo into state in order to access it later.

This is illustrated in the example posted to twitter by @gaearon: https://twitter.com/dan_abramov/status/953612246634188800?lang=en

What is the expected behavior?

Ideally (if it's not difficult to implement!) the getDerivedStateFromProps would also take the current (previous/old) props as an argument, something like:

getDerivedStateFromProps(nextProps, prevState, prevProps)

This would eliminate the need to constantly assign props to state purely for comparison purposes...

A quick look at the source doesn't make it clear to me how easy this would be though...

https://github.com/facebook/react/blob/4a20ff26ecfe9bc66941d79f7fce2c67be8ee236/packages/react-dom/src/server/ReactPartialRenderer.js#L456

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.3.0

[bvaughn]

Hi 😄

This idea was discussed in the RFC as well as in a follow-up issue filed in that repo (reactjs/rfcs/issues/19).

The short version is that the current API _does_ enable this, as shown in this example:

class ExampleComponent extends React.Component {
  // Initialize state in constructor,
  // Or with a property initializer.
  state = {};

  static getDerivedStateFromProps(nextProps, prevState) {
    if (nextProps.currentRow !== prevState.lastRow) {
      return {
        lastRow: nextProps.currentRow,
        isScrollingDown:
          nextProps.currentRow > prevState.lastRow,
      };
    }

    // Return null to indicate no change to state.
    return null;
  }
}

I felt that a nullable prevProps parameter would be potentially too confusing and using prevState for this was more intuitive.

[jharris4]

Right, as I mentioned in my original post, I know the current API does enable this, but it requires mixing props into the state (I had linked to a similar example to the one you just provided).

I know it's not the end of the world to mix the props into the state, but to me it has a bad code smell (particularly when you're only mixing props into state for the sole use of this function).

I'd be curious to know why you think the prevProps parameter would be confusing, particularly since the use of it would be entirely optional.

Thanks for linking the RFC issue (https://github.com/reactjs/rfcs/issues/19), I hadn't thought to look for discussion in the other repo! :-)

To summarize, we currently have a large number of React Components that currently utilize componentWillReceiveProps, and almost all of them compare several props with this.props.prop !== nextProps.prop.

When we migrate to using getDerivedStateFromProps we'll now have to push each and every one of those props into state, which in my opinion is not ideal.

It also means we'll have to go and sanity check the state of each of these components to make sure that we don't accidentally end up with prop vs state key collisions, and we have to make sure we don't mixup reading the key from state accidentally instead of props...

That last point is probably one of the best reasons I could put forward as to why I think it'd be cleaner (and arguably less confusing) to just pass the prevProps as an extra argument.

[bvaughn]

I'd be curious to know why you think the prevProps parameter would be confusing, particularly since the use of it would be entirely optional.

For the reasons previous mentioned on the RFC discussion thread I linked to in my reply above. (Have you read that thread?)

[jharris4]

(Have you read that thread?)

LOL. Yes, despite its length, I did scan it all, but didn't see a specific reference to why you think the prevProps parameter would be confusing.

[jharris4]

Sorry if I'm being obtuse, but why would passing in the prevProps lead to confusion?

Is it something related to the discussion thread surrounding PureComponent ?

[bvaughn]

Sorry, I wasn't trying to sound sarcastic. It was a serious question. 😄

It's just that this has literally been discussed already in the RFC and in the issue I linked to, and it doesn't seem productive to repeat the discussion here.

I deep-linked you to the previous conversation above (although it looks like GitHub is auto-collapsing the parent thread on page load, since there are so many comments, so that sucks).

Try loading the page again and searching for "amannn reviewed 18 days ago" then click "show outdated" to see the expanded comments.

[jharris4]

Ahhh! yeah, that was not obvious, it was indeed buried under the "show outdated". Thanks for your patience while I got orientated! :-)

So I now know that by "confusion" you were referring to the initial call to getDerivedStateFromProps where the prevProps do not yet exist...

My vote would still be to do that rather than force pollution of the state, but having now read the original thread, I see that this concern was already raised and that you've nonetheless committed to the API as is.

Maybe I'll look into creating some kind of utility HOC that at least removes the duplicated boilerplate required for moving props to state, and ideally even stores all stateProps under one state key to mitigate the risk of collisions. something like:

getDerivedStateFromProps(nextProps, prevState) {
  const  prevProps = prevState.__cloneProps;  
  if (!prevProps || prevProps.propA !== nextProps.propA || prevProps.propB !== nextProps.probB) {
    return {
      thingInState: "value",
      __cloneProps: cloneObjectWithKeys(nextProps, ['propA', 'propB'])
    };
  } 
  return null;
}

[bvaughn]

Yeah, no worries. Looks like a GitHub bug. I've reached out to them about it.

I appreciate the understanding. 😄

[catamphetamine]

Not having prevProps in the new getDerivedStateFromProps() function is really weird. I see no reasons for not passing prevProps argument to getDerivedStateFromProps(), nor I'd find it "confusing" in any way.

Well, whatever, I'll just store all prevProps inside state, something like this.state.props.

Unneedlessly verbose and bloated, but the library authors gave us no another way.

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 }
}

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

[TrySound]

@catamphetamine What is verbose there? Previous props are not passed for the same reason as you have to pass props to state in constructor. Actually your way won't work for the first call. Countries will be equal.

I usually just use "default" values

state = {
  prevCountry: null
}

static getDerivedStateFromProps(props, state) {
  if (props.country !== state.prevCountry) {
    return {
      prevCountry: props.country,
      derivedValue: ...
    }
  }
}

Yep, this is a bit verbose but with this you have more simple code with less conditions. You don't need to do null checks for prevProps, which is actually more verbose.

[catamphetamine]

@TrySound

What is verbose there?

This part is verbose.

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 }
}

Have the library authors given it a bit more thought they would have added the third prevProps argument to getDerivedStateFromProps() and everyone wouldn't have to add that bulky constructor state hack to work around that inconvenience.

Actually your way won't work for the first call.

No, it will work for the first call.

Countries will be equal.

And so it will simply not enter the if condition which is completely fine.

How this new getDerivedStateFromProps() function should have been really implemented is having the 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).

[TrySound]

Yep, this was considered.

https://github.com/reactjs/rfcs/pull/6/files#r156668036

[catamphetamine]

@TrySound Ok, I take your word for it. Didn't read that thread.

I mean, yeah, that would be a perfectly sane approach given that even the official docs explicitly state:

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.

But whatever, I'll take it any way you put it, just be done with it and move on.

[bvaughn]

I'm going to lock this issue because it doesn't seem like re-hashing this discussion is very productive at this point. 😄

The RFC has a lot of useful info on it. If there's uncertainty on why a decision was made, I'd suggest reading through it. (I know it's lengthy, but this was a big change.)