The minor release of 16.4 causes BREAKING changes in getDerivedStateFromProps

[mririgoyen]

According to semver, only non-breaking changes are supposed to go into minor and patch version releases. With the release of 16.4, you have made breaking changes to getDerivedStateFromProps. Our entire codebase, running perfectly on 16.3.2 is a dumpster fire as soon as we raise that dependency to 16.4.

The only thing in your CHANGELOG about this breaking change is:

Properly call getDerivedStateFromProps() regardless of the reason for re-rendering. (@acdlite in #12600 and #12802)

Please revert this change and save it for 17.0.0, or provide proper documentation to what this change actually entails so that its use can be adjusted by those who have already implemented getDerivedStateFromProps.

[gaearon]

Update: see my comment at the bottom of this thread for the conclusion.

TLDR: if this change broke your code when you moved from 16.3 to 16.4, your code was already buggy in subtle ways. The change in React 16.4 made these bugs in your product code occur more often so you can fix them. We don’t consider making existing bugs in your code reproduce more reliably to be a breaking change.

If you're coming from an issue in some third-party library that works with 16.4, but doesn't work in 16.3 — you're hitting the React bug. This issue is a complaint about this bug getting fixed in 16.4. However, we think fixing it was the right choice. We recommend everyone to upgrade to React 16.4 or later.

Again, for details, walkthroughs and demos, see my comment at the bottom of this thread.

---

I’m sorry this broke things for you. Can you show the relevant code please?

We’re aware this change can cause issues in rare cases, as documented in the blog post. Unfortunately not making this change also leads to buggy behavior, although less obvious and deterministic so maybe you didn’t bump into it yet. I can see your point that fixing it is “breaking” in your case, but I hope you can also see that fixing any bug can be considered breaking for the code that relied on it.

If your code relied on the old behavior but doesn’t work with the new one, it worked by accident. It’s our fault for overlooking this in the 16.3 release but it took about a month for us to discover this problem from bug reports at Facebook. The function takes two arguments: props and state. But it only runs when one of them updates. This didn’t really make sense and was an oversight in the initial implementation.

When we fixed that issue at Facebook (which, as we learned, was the reason behind numerous bugs), one component that relied on the buggy behavior broke (just like in your case). This was just one component out of a thousand or so. We decided that it’s better in the long term to make the fix than to allow more components to be written that rely on the bug.

Again, I’m sorry this caused issues for you. Seeing your code would be helpful.

[chase]

The documentation on the previous blog post seems to completely contradict the changes made in v16.4. Perhaps a note should be added to prevent confusion?

[gaearon]

@chase Can you be more specific as to where you see the contradiction? It fires more often but the examples in the post still work the same way.

[aweary]

This didn’t really make sense and was an oversight in the initial implementation.

@gaearon can you expand on this? The spec'd behavior in the RFC never mentioned this, so it seems like it was an issue with the initial API design and not it's implementation?

[jquense]

yeah i was also surprised here, the old behavior was more waht i'd have guessed the behavior would be from reading through the RFC

[gaearon]

The RFC does say:

Note that React may call this method even if the props have not changed.

We didn't want to over-specify when exactly this happens because there might be multiple reasons. setState was meant to be one of them but we missed this ourselves (in part because we left it vague in the RFC) and only realized the omission after seeing bug reports caused by this.

Again, the new behavior is critical to making getDerivedStateFromProps safe for async mode. If we didn’t do it, migration from componentWillReceiveProps would have largely been pointless.

None of the intended usage examples we provided in RFCs, blog posts, or docs would break with the new behavior.

[mririgoyen]

I'm not sure I understand the benefit or reasoning to have this method called on every single change.

We had to go back and write much more logic to handle the internals of all the getDerivedStateFromProps method calls after this change that feels more like boilerplate code the framework should be (and was before 16.4) handling. It just add more noise and bloat to our components.

The 16.3.2 version acted more in lines with the former componentWillReceiveProps, which this method replaced. In the pre-16.4 behavior, it was much easier to migrate from the deprecated componentWillReceiveProps to the new getDerivedStateFromProps.

The blog post provided by @chase states:

The new static getDerivedStateFromProps lifecycle is invoked after a component is instantiated as well as when it receives new props.

This is exactly what we expected and how it operated in 16.3. That is much cleaner in my opinion.

As a side note, and on a personal level, I never liked the deprecation of componentWillReceiveProps and have complained about it internally to many of my coworkers. However, I understand the side-effects that are trying to be edged out by changing methodologies on how the lifecycle works. That being said, if the React group feels that the new behavior introduced in 16.4 is how they wish it to work, I would like to suggest another lifecycle method is introduced more in line with how componentWillReceiveProps worked, or undeprecating that method and putting more protections around it to try and edge the issues you intended to prevent. Of course, I am not an architect, nor an employee of Facebook, or a member of the React development team, but the changes around the lifecycle in this particular area are extremely frustrating as a developer.

[gaearon]

If we want to continue this discussion let's start talking about specific code examples. If you have code that got broken by this, please show it and let’s talk about it.

[gaearon]

The 16.3.2 version acted more in lines with the former shouldComponentUpdate, which this method replaced. In the pre-16.4 behavior, it was much easier to migrate from the deprecated shouldComponentUpdate to the new getDerivedStateFromProps.

Sorry, but this seems like a significant confusion on your side. shouldComponentUpdate was never deprecated. It also has nothing to do with what getDerivedStateFromProps is for.

(That was in response to a typo, now edited)

[gaearon]

Also I'd like to ask everyone to keep in mind that we're not doing changes just to piss people off. We want what is best for apps in longer term, and we care about not adding extra boilerplate as deeply as you do. Again, we're sorry for the churn this caused.

We'll make another blog post about this next week. It seems like more people use getDerivedStateFromProps than we expected, and that is likely due to a misunderstanding about its purpose. For example, it might be that the code that relied on componentWillReceiveProps is always getting updated to getDerivedStateFromProps whereas memoization or lifting state up would have been more appropriate. We'll show more appropriate strategies that don't introduce the kind of boilerplate you're concerned with.

But to continue this discussion we need code examples. You can see otherwise it immediately turns abstract and vague.

[mririgoyen]

@gaearon No, no confusion, just poor copy/pasting. I immediately edited it. Thanks.

[gaearon]

Could you post an example of the code that got broken by the new behavior?

[mririgoyen]

Here is the simplest example I can provide from our morning of refactoring to support the update.

Before: (only called on prop changes)

static getDerivedStateFromProps(nextProps) {
    analyticsPageview(nextProps.documentId);

    return {
      source: nextProps.source,
      selection: {
        startBlockIndex: 0,
        startOffset: 0,
        endBlockIndex: 0,
        endOffset: 0
      }
    };
  }

After: (refactored because it is called every change)

static getDerivedStateFromProps(nextProps, prevState) {
    if (prevState.documentId !== nextProps.documentId) {
      analyticsPageview(nextProps.documentId);

      return {
        documentId: nextProps.documentId,
        selection: {
          startBlockIndex: 0,
          startOffset: 0,
          endBlockIndex: 0,
          endOffset: 0
        },
        source: nextProps.source
      };
    }

    return prevState;
  };

At this point, the documentId never will change. Previously, there was no reason to include it in the state. Additionally, we needed to wrap an additional logic statement around the entire thing.

This specific example is small and I can see the "no big deal" attitude that some people may give based on it but again, we have much larger refactors that this change caused.

[gaearon]

I believe the “before” code already had a bug (so 16.4 just surfaced it). Let me demonstrate it.

At this point, the documentId never will change.

It may be true that in your particular case this prop never changes. That’s a pretty fragile assumption from the component’s point of view—ideally it should be able to work with new props—but maybe it’s a reasonable intentional limitation of this component’s API.

Still, its parent component could re-render at any time (e.g. if it had setState call itself, or if one if its parents did). It could even be a parent component from a third-party library (e.g. a router that adds a setState call in a patch or a minor version).

It’s hard to tell whether this can happen or not without checking the source of every single component above in the tree. So even if this doesn’t happen right now in your code, the chances are high it might happen in the future when you’re working on a completely unrelated feature.

So why is that a problem? If any of the parents re-render, getDerivedStateFromProps would also get called again on your component. In your example, even with React 16.3, a parent re-render would reset the selection values though the document ID hasn’t changed. Your analytics call would also fire twice.

This is why the “before” code already had a bug, and the change in React 16.4 helped uncover it.

Even if none of the components above currently ever re-render, that’s an implicit assumption that makes every component with getDerivedStateFromProps fragile because such code relies on nothing ever updating above it. You can see this doesn’t really work with the promise of an encapsulated component model. People generally expect that it’s safe to add state to components above, or to move a component to a different tree.

That's exactly the kind of broken assumption that led to bugs at Facebook, and led us to do this change. React 16.4 calls it more often which surfaces such bugs that already exist in your code.

Conversely, getDerivedStateFromProps implementations that don’t contain this bug work correctly both in React 16.3 and in React 16.4.

Note how in the 16.3 blog post we explicitly demonstrated you need to keep previous values in the state for use cases like this, just like you ended up doing in the “after” example. Then you wouldn't have this problem now.

Note that additionally, getDerivedStateFromProps is intended to be a pure method. It’s not an appropriate place for side effects like the analytics call (use componentDidUpdate for them instead). You’ll get duplicate analytics events for every re-render. I’m sorry if this wasn’t clear from the docs. That wasn’t the main bug in the code but I figured I’d mention that.

Finally, there are also more subtle issues that would happen in async mode with code like this. Since the whole point of getDerivedStateFromProps was to migrate from async-unsafe patterns, it would be pointless to allow people to keep relying on it. If you didn’t fix this bug, you wouldn’t have gained anything from the componentwillReceiveProps migration.

[chase]

For what it's worth, none of my code broke after the update. It was specifically based off of this example on the blog.

Based on the changes mentioned in the newest post, I had assumed that this would no longer behave as expected:

  static getDerivedStateFromProps(nextProps, prevState) {
    // Store prevId in state so we can compare when props change.
    // Clear out previously-loaded data (so we don't render stale stuff).
    if (nextProps.id !== prevState.prevId) {
      return {
        externalData: null,
        prevId: nextProps.id,
      };
    }

    // No state update necessary
    return null;
  }

[gaearon]

I don’t see why that example would break with the update. Am I missing something? As far as I can tell the conditional check would be false on state-only updates, and it would return null.

[RoyalHunt]

Why I still have an error: http://i.imgur.com/fjFSDgq.png , when I do not have any componentWillReceiveProps in my project: http://i.imgur.com/cXl7Kad.png ?

[gaearon]

@royalhunt Please don’t use this thread to discuss a completely unrelated problem. If you believe it’s a bug please file a new issue with a reproducing example.

[chase]

I suppose the fact it looked so similar to what the blog post states breaks is what threw me off:

static getDerivedStateFromProps(props, state) {
  if (props.value !== state.controlledValue) {
    return {
      // Since this method fires on both props and state changes, local updates
      // to the controlled value will be ignored, because the props version
      // always overrides it. Oops!
      controlledValue: props.value,
    };
  }
  return null;
}

The similarity is superficial:

static getDerivedStateFromProps(nextProps, prevState) {
  if (nextProps.id !== prevState.prevId) {
    return {
      // Since this method fires on both props and state changes, local updates
      // to the controlled value will be ignored, because the props version
      // always overrides it. Oops!
      prevId: nextProps.id,
    };
  }
  return null;
}

[benwiley4000]

The given explanation for why the change was made is reasonable, it seems, but that's a separate issue from the fact this change was released without a major semver bump. 16.4 breaks what was generally understood to be the correct behavior in 16.3. I hear that there was some intentional ambiguity in the spec, but that doesn't change how folks understood gDSFP to work.

I'm glad none of my own code broke, but for a project as widely adopted as React it seems reckless to ship a breaking change under the guise of a "bugfix." I understand it may be inconvenient and out-of-step and bad marketing to release a new major version all of a sudden, but on the flipside this makes React's versioning less reliable and it makes me a bit more hesitant about upgrading to new React versions.

[RoyalHunt]

Actually I have some breaking problems as well with using apollo 1. But I solved this with the next code:

static getDerivedStateFromProps(nextProps, state) {
    const savedArticles = nextProps.data.articles
    if (savedArticles && !state.firstRender && savedArticles !== state.savedArticles) {
      return {
        savedArticles,
        firstRender: true
      }
    }
    return null
  }

Before it was:

static getDerivedStateFromProps(nextProps) {
    const savedArticles = nextProps.data.articles
      return {
        savedArticles,
        firstRender: true
      }
  }

[codeaid]

@gaearon I don't have any code examples (and I've not even installed 16.4 yet) but I just wanted to comment on what you said earlier...

It seems like more people use getDerivedStateFromProps than we expected, and that is likely due to a misunderstanding about its purpose. For example, it might be that the code that relied on componentWillReceiveProps is always getting updated to getDerivedStateFromProps whereas memoization or lifting state up would have been more appropriate.

I'm surprised you're surprised about this. I fully expected people to use this method simply because core methods on which, I'm sure, many relied upon previously are being removed. Namely those that allow us to tell when props change externally. I, personally, got the impression that this is almost the new componentWillReceiveProps from reading the docs.

As for me, my use case for getDerivedStateFromProps are components that receive objects to be edited from external sources (usually from Redux via mapStateToProps) and store that data in the local state (which has to be done as you can't edit property values directly). They also usually some onSave(newData) type callbacks (mapped via mapDispatchToProps) that are triggered when the updated data needs to be persisted. Those callbacks result in new data being pushed into the current component's props and local state having to be updated.

Before getDerivedStateFromProps existed I had to check for presence of and changes to this.props.something in both - constructor and componentWillReceiveProps - because data could be available during the construction, or alternatively it might've been necessary to load it first, meaning it would become available through one of the eventual componentWillReceiveProps calls.

getDerivedStateFromProps is obviously a godsend in that regard and makes the code so much cleaner and neater and I personally can't see any other way to implement behaviour similar to what I described above. It would definitely be interesting to see how memoization or lifting state up would have been more appropriate as you said. I'm pretty sure I looked into those options but couldn't find a solution that would work for these use cases. componentDidUpdate can be used in some occasions but sometimes it's too late and things need to be checked before something gets changed and not after.

[bvaughn]

I hear that there was some intentional ambiguity in the spec, but that doesn't change how folks understood gDSFP to work.

Our original implementation was flawed. For what it's worth, the RFC spec for getDerivedStateFromProps did make mention of this:

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.

And our examples/recipes recommended comparing new and previous prop values before updating state- but we failed to community this warning clearly enough in the API reference docs, and I apologize for that.

---

I, personally, got the impression that this is almost the new componentWillReceiveProps from reading the docs.

We need to do a better job of communicating when getDerivedStateFromProps is appropriate and when other techniques would be better. (See this tweet for an example.) We plan to publish another blog update in the next week or two that covers this in more detail.

It would definitely be interesting to see how memoization or lifting state up would have been more appropriate as you said.

We will be sure to include several examples in the upcoming update about this.

[codeaid]

@bvaughn Looking forward to the article. One thing I'd suggest, and it's a comment that a few of my colleagues have expressed - lots of the important documentation actually seems to be in blog posts and not the API pages themselves. I would personally prefer to see documentation expanded and blog posts just linking or quoting parts of it. As it stands, there are things (logic, suggestions, exceptions) which are only available in blog posts.

[bvaughn]

That's a good point, @codeaid. It's can be difficult to strike the right balance between too much and too little detail, but I think our current reference section definitely needs some work.

We hope to eventually create a "recipes" section on the site that shows common tasks and our suggested patterns. Unfortunately there are only a couple of us though, and this sort of thing is pretty time consuming.

But I appreciate the feedback. We'll try to keep it in mind as we make small edits over the next couple of weeks.

[codeaid]

@bvaughn Ask Mark to give you more people! It's ridiculous that a library this popular globally is getting so few resources :)

[benwiley4000]

Assign Mark a few code reviews at least!

[dubbha]

Am I the only one who now just can't grasp the naming and the param names of this method? It's called getDerivedStateFromProps and the params are (nextProps, prevState), and it totally made sense in 16.3 - we were literally getting derived state from props, on the new props, thus having nextProps and prevState as params. Yeah, it was kind of a new componentWillReceiveProps, but at least the naming made sense, particularly when I was going through this diagram two days ago. Now when the same method will fire on setState() and forceUpdate(), I am totally confused. Why do I need it on setState()? How do I get the newState in this case? How is a prevState useful in this case? What am I deriving from what? And why do I need it on forceUpdate()?

[gaearon]

Would it help to say that arguments are just called props and state and represent current props and state?

I thought prefixes would be helpful but I see now they’re more confusing.

Why do I need it

I don’t understand this question. The use cases haven’t changed. If you needed this method in 16.3 and your code doesn’t have bugs (such as the one described above), it should also work in 16.4. So if you “need it” it’s for the same reason you “needed it” in 16.3.

If you don’t need it that’s cool too—it’s not intended to be commonly used. As I already noted a few times in this thread it’s hard to say whether you need it if you don’t show a particular snippet of code.

[bvaughn]

No, you're not the only one, @dubbha!

Andrew and I were talking about this earlier this week and decided to update the docs to just be props and state like in the most recent blog example but I guess neither of us have made this change yet. I will do it now!

Edit: Docs updated via reactjs/reactjs.org/pull/910

[gaearon]

@RoyalHunt

Actually I have some breaking problems as well with using apollo 1.

Looking at your “before” code in https://github.com/facebook/react/issues/12898#issuecomment-391847365, it has the exact same bug as I described in https://github.com/facebook/react/issues/12898#issuecomment-391782913.

So here, too, React 16.4 didn’t break your code. It helped you find the bug that already existed (previously unnoticed) in your code and manifested itself less deterministically.

Sounds like a good thing to me.

[Tolgeros]

Am I correct in understanding that an updated version of this diagram would have gDSFP stretching to the right, such that setState will now also trigger it (but not forceUpdate)?

Also, would this be a sensible way, in general, to mimic the 16.3 behavior in 16.4? As in, I don't want gDSFP to do anything if the props did not change. Can I just do a basic object reference equality comparison of the props object (rather than comparing specific props) to solve the problem in a general sense?

Before:

static getDerivedStateFromProps (nextProps, prevState) {
   return {
      value: nextProps.value
   }
}

After:

static getDerivedStateFromProps (props, state) {
   if (state.prevProps !== props) {
      return {
         value: props.value,
         prevProps: props
      }
   } else {
      return null
   }
}

To add some background, these methods would be used by components that will do setState of value and call a props.onChange method in the setState's callback parameter to bubble up the change to the real source of truth (which may be some top level state or Redux store). It needs a copy of 'value' kept in its local state, for user experience reasons, but if changes come from above it needs to be able to respond to it.

_handleChange = (event) => {
   this.setState({value: event.target.value},
      () => this.props.onChange(this.state.value)
   );
}

[dubbha]

@gaearon, @bvaughn, first of all thank you for your answers.

Would it help to say that arguments are just called props and state and represent current props and state?

Yes, it helps, at least it is more obvious that what I am getting as the second argument in case of setState() is the current, newState, not the prevState instead.

The use cases haven’t changed. If you needed this method in 16.3 and your code doesn’t have bugs (such as the one described above), it should also work in 16.4. So if you “need it” it’s for the same reason you “needed it” in 16.3. If you don’t need it that’s cool too—it’s not intended to be commonly used. As I already noted a few times in this thread it’s hard to say whether you need it if you don’t show a particular snippet of code.

When asking the "why do I need it" question I was mostly trying to think of the new use cases for the method. It covers the new props use cases perfectly. I was thinking of a use case for the setState() and forceUpdate(). There were no such use case before of course, so there's no code to show and nothing is broken. I am just not getting how this method could be useful in the latter two cases. Just can't think of the use cases. And what I would be deriving from what.

@Tolgeros

Am I correct in understanding that an updated version of this diagram would have gDSFP stretching to the right, such that setState will now also trigger it (but not forceUpdate)?

Nope, it also fires on forceUpdate() (but skips shouldComponentUpdate): https://codesandbox.io/s/wk7qxnvk38

[qndrey]

For example, in this case, getDerivedStateFromProps triggers in each setState, thus I have always default value, which is prop value (15).

class RangeInput extends React.Component {
  state = {
    value: this.props.value,
  }

  static propTypes = {
    value: PropTypes.number.isRequired,
  }

  static getDerivedStateFromProps(nextProps, prevState) {
    if (nextProps.value !== prevState.value) {
      return {
        value: nextProps.value,
      }
    }

    return null
  }

  onChangeRange = e => {
    this.setState({ value: parseFloat(e.currentTarget.value) })
  }

  render() {
    const { value } = this.state

    return (
      <div>
          <input
            type="range"
            min="0"
            max="100"
            step="0.5"
            value={value}
            onChange={this.onChangeRange}
          />
        </div>
    )
  }
}

ReactDOM.render(<RangeInput value={15} />, document.querySelector("#app"))

is here another workaround?

[gaearon]

@Tolgeros in https://github.com/facebook/react/issues/12898#issuecomment-391946524

Also, would this be a sensible way, in general, to mimic the 16.3 behavior in 16.4?

Yes, I think this looks equivalent to me. I don’t recommend writing code like this though. It is fragile. Let me expand on that.

It needs a copy of 'value' kept in its local state, for user experience reasons, but if changes come from above it needs to be able to respond to it.

Just to clarify we’re on the same page: do you realize that just re-rendering parent for any reason whatsoever counts as “change from above”?

For example, if you wrap your component in some popup container that passes closePopup as a prop to its child, and that function is different on every render (so equality checks in Redux containers won’t stop it), and the popup calls setState, your component will re-render and blow away the user input.

I think any code that tries to distinguish “change” from above by comparing whole props objects instead of individual props (or, in the past, just relying on when the method gets called) will lead to bugs like this.

To implement something like you described, it would make sense to me if you at least determined the change by comparing some explicit marker coming from Redux store (e.g. an ID or version field) that explicitly gets incremented when you intend to blow away the local state.

Do you see what I mean?

@dubbha in https://github.com/facebook/react/issues/12898#issuecomment-391990732

Yes, it helps, at least it is more obvious that what I am getting as the second argument in case of setState() is the current, newState, not the prevState instead.

Well yes, the only reason we called it prevState in the docs is because you are returning some state already. So the input state is definitely “older” than what you are creating right now. I was worried people would attempt to mutate the state object if it was just called state.

When asking the "why do I need it" question I was mostly trying to think of the new use cases for the method.

Maybe I’m wrong but I don’t think the change is intended to cover new use cases. It is intended to uncover dormant bugs in existing use cases, as we’ve seen already three times in this thread:

• https://github.com/facebook/react/issues/12898#issuecomment-391847365
• https://github.com/facebook/react/issues/12898#issuecomment-391782913
• And the issue described in this comment

So the use cases are the same, and if you use the method in supported ways, you don’t need to make adjustments. If your code has bugs, they became more prominent, and you need to fix the code to follow the supported patterns (like in the documentation and blog posts).

@andrewBalekha in https://github.com/facebook/react/issues/12898#issuecomment-392001463

For example, in this case, getDerivedStateFromProps triggers in each setState, thus I have always default value, which is prop value (15).

Your code also has the same problem as in three cases above that I described, even in React 16.3.

If the user inputs something but then parent component re-renders for a different reason, you will blow away user input (because it won’t match the state).

For every component you need to decide if some value is controlled or uncontrolled. This is part of your component’s API design.

If it is controlled then you should use the value from props, and not attempt to mirror it in state. When you want to change something, you need to ask the parent to do so. This is called “lifting state up” and is described in detail in documentation. In this scenario you don’t need getDerivedStateFromProps.

If it is uncontrolled then you can take a prop like defaultValue for the first render. But from that point you should use local state to keep the value, and re-rendering from above shouldn’t reset it. In this scenario you don’t need getDerivedStateFromProps either.

Combining controlled and uncontrolled behavior (using getDerivedStateFromProps or other means) is possible but extremely error-prone. I strongly suggest against it. You can see from my three previous bug descriptions in this thread that this leads to the same mistake: a completely unrelated parent re-render will blow away your state.

So what do you do instead if you want to blow away the state when something changes in the parent? You need to figure out what exactly should serve as a trigger for blowing away the local state. For example maybe in some parent there is a state like currentItemID that changes. In that case, you can either:

• Pass that state down to the child so it can use it in getDerivedStateFromProps and decide to blow away the existing local state.
• Or you can give key={this.state.currentItemID} to the child so that when the ID changes, it is forced to be unmounted and then mounted again with new props. This will ensure the “clean slate” without getDerivedStateFromProps but will blow away existing DOM. And in fact that might be exactly what you need if conceptually the “current item” has changed and you don’t want to preserve things like focus, uncontrolled input state, or scroll positions.

Does this explain the problem a little bit? I’m not sure if my explanations are clear enough because I keep describing the same problem in the application code but every new example in this thread demonstrates it again as if it was desired behavior.

[qndrey]

@gaearon, thanks I've got my problem now.

Here is an example where my code is failing in 16.3 as well https://codesandbox.io/s/j809y1llv

[karolk]

Simpler example of the issue: https://jsfiddle.net/6fme1y69/2/ It seems like coming from componentWillReceiveProps in 16.2 to getDerivedStateFromProps in 16.3 it was very easy to refactor the code to something that stopped working properly in 16.4.

[gaearon]

@karolk

This code already didn’t “work properly” in React 16.3. I believe I explained in detail why code like this is buggy in several comments above. Did you get a chance to read them yet? Or am I missing the reason this example is different and doesn’t already suffer from the issues I described (even in 16.3)?

I’m currently away from keyboard but I can create a demo fiddle when I’m at work if my comments are unconvincing.

[bvaughn]

@karolk Dan already mentioned why this wouldn't work in a few comments above. Essentially it boils down to:

if the user inputs something but then parent component re-renders for a different reason, you will blow away user input (because it won’t match the state).

For your example, even in React 16.3, if your Test component re-rendered because a prop _other than text changed (e.g. an onChange type callback), then your state would be overridden. This mixing of props and state has awkward/broken edge cases, even before getDerivedStateFromProps.

I'm going to quote from Dan's comment right above because it's a great summary:

---

For every component you need to decide if some value is controlled or uncontrolled. This is part of your component’s API design.

If it is controlled then you should use the value from props, and not attempt to mirror it in state. When you want to change something, you need to ask the parent to do so. This is called “lifting state up” and is described in detail in documentation. In this scenario you don’t need getDerivedStateFromProps.

If it is uncontrolled then you can take a prop like defaultValue for the first render. But from that point you should use local state to keep the value, and re-rendering from above shouldn’t reset it. In this scenario you don’t need getDerivedStateFromProps either.

Combining controlled and uncontrolled behavior (using getDerivedStateFromProps or other means) is possible but extremely error-prone. I strongly suggest against it. You can see from my three previous bug descriptions in this thread that this leads to the same mistake: a completely unrelated parent re-render will blow away your state.

[karolk]

@gaearon I think my example is slightly different. I updated the fiddle to demonstrate the behaviour I was going for. I was trying to have a box where you can type or paste anything but only a value meeting certain criteria ends up in a store and comes back as a prop. I am aware that this can be achieved differently and it is maybe using gDSFP in a way that's too similar to cWRP. https://jsfiddle.net/6fme1y69/3/

My point with this is that you can go wrong even if you assume gDSFP can fire many times.

[karolk]

@bvaughn I went for a combination of parent controlled and state but this seems to depend on implementation detail. I don't think there's anything I don't get in @gaearon's explanation. Thank you both for your time, I appreciate quick response here and on Twitter.

[bvaughn]

@karolk Did you have a chance to read my reply or the sections of Dan's reply that I highlighted? (Sorry, our comments were posted at the same time. 😄)

It seems like you're aiming for an uncontrolled component, in which case we strongly recommend that you don't try to mirror props in state like your component is doing. Instead, you can initialize state with a default value from props like so:

class Test extends React.Component {
  constructor(props) {
    super(props);

    // Read default text from incoming props (store),
    // But then don't try to mirror it to state.
    this.state = {
      text: props.defaultText
    };
  }

  changeText(newText) {
    this.setState({ text: newText }, () => {
      // Notify store of text change if some criteria is met.
      if (this.state.text.match(/\d+/g) === null) {
        dispatch(this.state.text);
      }
    });
  }

  render() {
    return (
      <input
        onChange={({ target }) => this.changeText(target.value)}
        value={this.state.text}
      />
    );
  }
}

Even for React 16.2 (before there was getDerivedStateFromProps) mirroring props and state in the way your fiddle shows was not recommended because it can result in a confusing API or confusing runtime behavior:

• What's the source of truth when the two disagree?
• Unexpected re-renders or changes to unrelated props like callback functions or inline styles (which are often re-created inline during render) could blow away state unintentionally.
• If state is reset to props every time the component re-renders, it is entirely unnecessary and you should just use the props value instead.
• If state is only updated when the specific field in props changes, then the parent component has no way to forcefully reset to the previous props value without rendering twice (first to change the value arbitrarily and again to change it back to the target).

[gaearon]

I’m going to close this. In conclusion:

• If this change exhibits bugs in your code in React 16.4, your code was already buggy in React 16.3 in more subtle and less deterministic ways.
• To support this claim, I refer you to five examples posted in this thread https://github.com/facebook/react/issues/12898#issuecomment-391781205, https://github.com/facebook/react/issues/12898#issuecomment-391847365, https://github.com/facebook/react/issues/12898#issuecomment-391946524, https://github.com/facebook/react/issues/12898#issuecomment-392001463, https://github.com/facebook/react/issues/12898#issuecomment-392048949 which are all already buggy in React 16.3 as I explained in my analysis in https://github.com/facebook/react/issues/12898#issuecomment-391782913 and https://github.com/facebook/react/issues/12898#issuecomment-392035163.

If you’re not convinced by my explanation, let’s take a demo from https://github.com/facebook/react/issues/12898#issuecomment-392080509.

Indeed, at the first glance seems like in the version using React 16.4 you can’t edit anything, but in the version using React 16.3 the code runs fine. So is the original code free of bugs?

No, it’s not. Here is a fiddle using React 16.3 where we re-render the parent component every second. You can see that the input resets every second:

That clearly wasn’t intended. And all examples in this thread that got broken by React 16.4 already have the same exact bug. Whenever a parent re-renders, the state gets unintentionally reset. And it’s pretty hard to debug because components are decoupled, and whether you see this bug or not depends on how “lucky” you are.

React 16.4 helped surface this bug in your code so you can fix it, instead of keeping the app broken in cases that reproduce less reliably.

As we mentioned, we’ll follow up with another post (edit: it's live!) that describes when (not) to use getDerivedStateFromProps. I already shared some thoughts on component API design part of this in https://github.com/facebook/react/issues/12898#issuecomment-392035163.

But we maintain that making an existing bug in your app reproduce more consistently is not a breaking change, and is a helpful way to help you find and fix that bug. We are sorry for not being clear enough in the documentation and our guidance for using this method, and we’ll amend that by posting more in the coming week. (Edit: the post is live!)

Finally, I agree this method is kinda hard to wrap your mind around. I don’t think it’s the flaw of this API, it’s just that state-based-on-props was always hard to think about but this complexity was “hidden” due to imperative nature of componentWillReceiveProps. So even if bugs like this existed in your apps you might not have been able to reliably reproduce them every time.

The new method makes the complexity of what you’re trying to do explicit. The verbosity helps you realize that perhaps it’s not a good solution for your particular problem, and being more clear whether a value is controlled or uncontrolled, and lifting state up when necessary, will be better both from readability and correctness perspectives. Again, we’ll follow up with more guidance on that.

Thanks to everyone for the discussion! I hope this was helpful, and if I missed anything please let me know.

[gaearon]

@goyney

I couldn’t help noticing you downvoted my last comment. Is there something in my explanations that’s not clear or that seems wrong? I tried my best to explain why the change uncovers existing bugs (making existing broken codepaths more reproducible), and why code that didn’t have bugs with the old behavior also doesn’t have them with the new behavior (so it’s not a “breaking” change). Is something still unsatisfactory and could be better? What can we do to improve aside from publishing a blog post (as we plan to)?

I understand it’s frustrating when a version update leads to seeing bugs. But if those bugs already existed in your app, perhaps it’s a consolation that now you see them clearly every time, and have an opportunity to fix them?

[mririgoyen]

You broke semver, plain and simple. I understand your explanations and I understand your reasoning and do not disagree with them. But, when I yarn and get the latest minor release, my stuff shouldn't stop working because of a minor or patch version dependency change, plan and simple.

While the changes were good intended, that is my major sticking point in all this.

[sophiebits]

@goyney Every bug fix has the potential to break your code if you were relying on the buggy behavior. It's true that this bug fix is a little more likely than most to break code, but there isn't a hard line between "bug fix" and "breaking change"; it's much more of a spectrum. When we looked at this we felt it was closer to a bug fix.

[gaearon]

Semver is a social contract. I think it’s not as clear cut as it may seem. Here’s code that may randomly break and then “unbreak” on every single React release:

if (ReactDOM.render.toString().length > 330) {
  throw new Error('oops');
}

Does it mean we can never change render() implementation because any possible change would break semver? Probably not.

It’s a silly example of course. What about code like this?

<div onGotPointerCapture ={() => { throw new Error('no'); }} />

Technically it doesn’t do anything in React 16.3, but throws an error in browsers that support in React 16.4. Does that mean that adding support for pointer events was a breaking change?

This code also looks silly although maybe this version is a tiny bit less far-fetched:

const handlers = {
  onGotPointerCapture() {
    const x = somethingThatReturnsNull();
    x.foo();
  }
}

// somewhere
<Foo handlers={handlers} />

// ...
//  in Foo:

<div {...this.props} />

Maybe somebody could write something like this assuming that pointer events were already supported, and didn’t notice the warning. This code would break with 16.4 release.

I think it’s still implausible but we can argue that for any kind of change, there is a chance that there exists some code that happens to rely on the old behavior. If we wanted to respect semver in the strictest (and almost mathematical) sense then we’d have to make every single release major. Which of course defeats the purpose of semver.

So, coming back to my original point, semver is a social contract. We try hard not to break things that weren’t broken. But given the choice between “break already broken code more strictly and ensure more broken code doesn’t get written” and “keep broken code working in some cases, but allow more broken code to be written” it’s less clear cut. In this case, we decided that it’s worse for the ecosystem to endure the pain of fixing it later (and in the meantime live with bugs that are hard to reproduce) than make existing hidden bugs apparent. And this change does fix other categories of bugs by itself as well, so it’s not just a “make existing bugs more prominent” scenario.

I can see arguments for doing it the other way but I hope you can see ours too.

[codeaid]

I've read most of the comments in this thread but I'm still not quite sure if I'm understanding the planned changes correctly.

From what I understand instead of only being triggered when external properties are changed, getDerivedStateFromProps will now get triggered on every external AND internal update. Am I right? if that's the case then what is the alternative for actually only listening for external changes without having to compare props and state all the time?

You're saying if there were bugs in 16.3 they will surface in 16.4, however, I don't see how the use case I described in my comment earlier is covered or even achievable.

Just to recap my problem - there a top level components that "talk" to Redux and get state data pushed into them via props (using mapStateToProps) when Redux receives new data. Because these components are pretty much top level components there is no way for me to lift the state meaning I have to accept props AND have them stored in the local state so that I can edit them. Therefore the requirement is to be able to tell when something changed externally so that I can replace my local state. I don't care if I blow user data away because that's exactly the point - to replace local state with fresh data from the API.

I suppose we could say that in a way new props coming in from Redux can be compared to the parent component rerendering - you blow away internal state, however, that is the whole point in these components I have - you edit stuff, trigger an action, make an API call and get new data from Redux that has to replace the current state. Oftentimes componentDidUpdate is too late in the lifecycle so that's no good but if getDerivedStateFromProps will now get triggered on any change then I'll never be able to tell when something is about to change.

I encountered this problem when updating components from componentWillReceiveProps to getDerivedStateFromProps but with the way the latter one worked it was a logical change. Now I'm not even sure how to proceed.

[gaearon]

you edit stuff, trigger an action, make an API call and get new data from Redux that has to replace the current state

How do you determine that data coming from above is “new”? I don’t understand this part.

Judging by your description, you don’t compare any IDs or anything. So you’re just hoping that your components never get re-rendered unless something specific happens on the Redux side (e.g. a data fetch). This is a very fragile assumption.

As far as I can tell adding a single always-changing value to the mapStateToProps result (e.g. something: {}) will cause your component to always re-render on any action (not just the ones you care about), and always reset the state. Isn’t that bad? Adding a property or selecting more data from Redux (that might change more often than “top-level” data you’re selecting) shouldn’t break your component.

In other words, you seem to be relying on a performance optimization (React Redux currently bails out of rendering shallowly equal props) for correctness. This sounds like it will become a bigger problem later on (e.g. if React Redux stops using shallow equality checks everywhere).

It’s hard for me to say more without seeing the code but that’s the basic idea.

How do you solve this issue correctly? I believe I gave some pointers in https://github.com/facebook/react/issues/12898#issuecomment-392035163.

If you need this kind of behavior, you need some explicit marker that tells you the data is actually “new” (e.g. ID or version field is different) so it needs to be replaced. Your components should be resilient to being re-rendered.

Again, seeing a sandbox with a minimal code example would help.

[mririgoyen]

Semver is a social contract and it is a contract a company like Facebook should probably be following, especially with such a huge community of developers utilizing a, dare I say, standard in front-end development at this point in time.

You had functionality in a non-alpha/non-beta release out in the wild for two months. You had cited documentation stating the lifecycle's behavior. You made claims of componentWillUpdate deprecation and suggested developers start moving to the new modal as soon as possible to prepare for the eventual removal of the deprecated code.

With all of this, I would assume this means you had proper unit and contract testing for each piece of the lifecycle, including getDerivedStateFromProps. I can almost bet that at some point during 16.4's development tests were added or changed because the behavior changed here. In fact, you can see test file changes in both https://github.com/facebook/react/pull/12802 and https://github.com/facebook/react/pull/12600.

That should have been a red flag that a breaking change was being made. A change to the fundamental lifecycle of React is a big deal.

Was it really a bug? Yeah, I think based on the discussions in this thread and your explanations, it should be classified as an unintended behavior. But this wasn't a launch-day slip up. This was two months into the release of 16.3. This is a breaking change. It warranted more of a heads-up to the community or it should have been saved for 17.0. This one change wasted the collective community probably several hundred hours of productivity time to figure out why their code just stopped working. I know it took two developers at least two hours to track down why everything just quit working since this specific change omitted no errors.

I also understand that sometimes accidents happen, when a breaking change is pushed unintentionally. But everything you've stated in this thread indicate that this was intentional and you knew what you were doing. To me, that is extremely unprofessional and good way to upset your base of users. Something like this, especially if it happens more than once, can easily push someone to a competing framework.

Listen, I like React. It took a long time for me to ramp up on it and feel really comfortable with it. But I like it. I'm not going to go bashing it because of stuff like this. It's small, it's petty, and frankly, I have code I need to be writing and arguing with people over the interwebs is not something I like to do.

What I am asking you is to be more vigilant on how and when you release breaking changes in the future.