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