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.

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

The minor release of 16.4 causes BREAKING changes in getDerivedStateFromProps #12898

Closed
goyney opened this Issue May 24, 2018 · 104 comments

Comments

Projects
None yet
@goyney

goyney commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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

This comment has been minimized.

chase commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

@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 comment has been minimized.

Member

aweary commented May 24, 2018

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

This comment has been minimized.

Collaborator

jquense commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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.

@goyney

This comment has been minimized.

goyney commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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.

@goyney

This comment has been minimized.

goyney commented May 24, 2018

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

@gaearon

This comment has been minimized.

Member

gaearon commented May 24, 2018

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

@goyney

This comment has been minimized.

goyney commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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

This comment has been minimized.

chase commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

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

This comment has been minimized.

RoyalHunt commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 24, 2018

@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

This comment has been minimized.

chase commented May 24, 2018

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

This comment has been minimized.

benwiley4000 commented May 24, 2018

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

This comment has been minimized.

RoyalHunt commented May 24, 2018

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

This comment has been minimized.

codeaid commented May 24, 2018

@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

This comment has been minimized.

Contributor

bvaughn commented May 24, 2018

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

This comment has been minimized.

codeaid commented May 24, 2018

@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

This comment has been minimized.

Contributor

bvaughn commented May 24, 2018

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

This comment has been minimized.

codeaid commented May 24, 2018

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

@benwiley4000

This comment has been minimized.

benwiley4000 commented May 24, 2018

Assign Mark a few code reviews at least!

@dubbha

This comment has been minimized.

dubbha commented May 24, 2018

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

This comment has been minimized.

Member

gaearon commented May 25, 2018

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

This comment has been minimized.

Contributor

bvaughn commented May 25, 2018

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

@tankgod

This comment has been minimized.

tankgod commented Jul 4, 2018

image

@tankgod

This comment has been minimized.

tankgod commented Jul 4, 2018

This writing will lead to an infinite loop, always asynchronous request, how to solve this problem?

@gaearon

This comment has been minimized.

Member

gaearon commented Jul 4, 2018

As explained both in API reference for componentDidUpdate and blog post, you should always make a comparison in componentDidUpdate to decide whether to do something.

componentDidUpdate(prevProps, prevState) {
  if (prevProps.something !== this.props.something) {
    // do something
  }
  if (prevState.somethingElse !== this.state.somethingElse) {
    // do something else
  }
}
@tankgod

This comment has been minimized.

tankgod commented Jul 23, 2018

I have a piece of code, as follows:

addFiled(d: any) {
    const {factor} = this.state;
    const item = [$utils.generateMixed(6), d];
    factor.push(item);
    this.setState({factor});
}
componentWillReceiveProps(nextProps) {
    if (this.props.mode !== nextProps.mode) {
        if (nextProps.mode.type === 'edit') {
            this.setState({factor: []}, () => {
                // This is a loop
                mu.each(mu.prop(nextProps, 'mode.data.anys'), res => {
                    this.addFiled(res);
                });
            });
            if(mu.prop(nextProps,'mode.data.source.length')){
                this.setState({
                    origin: 'source'
                })
            }else{
                this.setState({
                    origin: 'platforms'
                })
            }
        }else{
            this.setState({
                factor: [[$utils.generateMixed(6), []]],
                origin: 'platforms'
            });
        }
        this.setState({
            visible: true
        });
    }
}

How do I change it with getDerivedStateFromProps?(I tried using ‘getDerivedStateFromProps’ and ‘componentDidUpdate’, but it went into an infinite loop.I am very annoyed)

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Jul 23, 2018

I tried using ‘getDerivedStateFromProps’ and ‘componentDidUpdate’, but it went into an infinite loop.

Please show what you tried.

@tankgod

This comment has been minimized.

tankgod commented Jul 24, 2018

I have solved this problem, thank you anyway.

@lvl99

This comment has been minimized.

lvl99 commented Aug 16, 2018

I'm still a bit perplexed on this myself.

While I understand the idea of "controlled" and "uncontrolled" data, I've got a few places where I want it to be both.

I have two examples (both have the same code, one using React 16.3 and the other React 16.4) with two intentions:

  • Control if a button is selected on or off (ButtonSelectable)
  • Change an object's contents based on form inputs (EditObject)

React 16.3: https://codesandbox.io/s/9lopjp809o
React 16.4: https://codesandbox.io/s/rrxjpn7p6m

In both intentions I want to make a copy of the external prop value and save it internally. Equally, I want any external changes to overwrite the internal state. The ButtonSelectable is a silly example — in most cases this should always be controlled — however in the EditObject one, I want to store a copy of the object in the component's state and manipulate that so the component's view is correctly updated, then publish my changes to the parent component. If the external prop object changes (let's say another component updates the object), I want those changes to overwrite what's in the internal state.

I don't doubt that there's probably an anti-pattern to the React 16.3 getDerivedStateFromProps working like componentWillReceiveProps, but I feel like that lifecycle method is still a good thing to include in future updates. I'm amiable, but I'm still just trying to understand the implications of the anti-pattern and how this new behaviour is better.

@gaearon

This comment has been minimized.

Member

gaearon commented Aug 16, 2018

@lvl99 Did you get a chance to read the blog post?

@lvl99

This comment has been minimized.

lvl99 commented Aug 16, 2018

@gaearon I did, but still wanting that sweetness of the componentWillReceiveProps behaviour. I attempted something like (warning -- mildly untested code ahead, think of it more as pseudo-code):

// ...
import memoize from "memoize-one";

export default class EditObject {
  // ...
  
  getObject = memoize((propsObject, stateObject) => {
    if (propsObject && propsObject !== stateObject) {
      return propsObject
    }
    return stateObject
  })
  
  // ...
  
  render() {
    let useObject = getObject(this.props.object, this.state.object);

    // ..
  }
}

But the problem that I'm finding is that I just don't want to compare both objects on each render (because when this.state.object changes it'll always then revert to this.props.object, but that's prob more a fault of my logic), just only when this.props.object changes. Would I perhaps use something like memoization to locally store the propsObject itself and then compare/setState when/if it changes? I wonder, is it "legal" to use setState in the memoised function too?:

getObject = memoize(propsObject => {
  if (propsObject !== this.state.object) {
    this.setState({
      object: propsObject
    });
    return propsObject
  }
  return this.state.object
})

I realise I've written a comment where I'm working stuff out at the same time. I'll modify my Codesandboxes to play around and get back to you...

@gaearon

This comment has been minimized.

Member

gaearon commented Aug 16, 2018

@lvl99

I don't think your use case is memoization?

What you're trying to do seems to be covered here:

https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#anti-pattern-erasing-state-when-props-change

Including why it's fragile and probably not a good idea.

@lvl99

This comment has been minimized.

lvl99 commented Aug 16, 2018

@gaearon Yeah, my approach is definitely not the right idea. However, just trying to figure out what the alternative is when wanting to sandbox some information that then may change due to other external side-effects. Thought it was still relevant to have something like componentWillReceiveProp, but perhaps that's just not the right idea as well. Just need to figure out the right path.

@gaearon

This comment has been minimized.

Member

gaearon commented Aug 16, 2018

Here's an updated example with 16.4 that works: https://codesandbox.io/s/m4km18q38j

My changes (compared to the broken 16.4 version you posted are):

  • Removed unnecessary state and getDerivedStateFromProps from ButtonSelectable. It's now fully controlled.
  • Removed unnecessary getDerivedStateFromProps from EditObject. Instead of mirroring object in state, I separated this.props.object (coming from above), and this.state.overrides (locally edited fields). I don't attempt to "sync" them. Instead, I merge them in the render method: {...this.props.object, this.state.overrides}. When I want to confirm edits, I send the merged objects upwards, and the parent component replaces its state.
  • I need to reset EditObject's overrides when we switch between "different" objects in the parent. However, as mentioned in the blog post, it is brittle to do so by comparing next and previous props. Even with componentWillReceiveProps this solution doesn't work very well (what if you need to update the object above for unrelated reasons but not reset the form state?). Instead, I added an id field to the objects. Now I can reset the overrides state using the "uncontrolled component with a key" approach: <EditObject key={this.state.object.id} />.

Hope this is helpful!

@lvl99

This comment has been minimized.

lvl99 commented Aug 16, 2018

@gaearon very helpful! Thanks for the update and the clear breakdown. Much appreciated.

The use of the overrides and the id is very cool. Thanks for taking the time 🙏

@drcmda

This comment has been minimized.

drcmda commented Aug 23, 2018

@gaearon is there anything the react team can do to warn users when they install 16.3.x? The newer gDSFP behaviour changes component fundamentals, for instance in the transition component in react-spring. Hence i get issues posted for it every now and then, for instance this one: drcmda/react-spring#198

gDSFP is assuming it's going to be called by setState (which is used by transition when exit-out transitions are completed). As a direct result old transitions are stuck in limbo in 16.3.x until a new render pass begins.

@gaearon

This comment has been minimized.

Member

gaearon commented Aug 23, 2018

I don't think this is a serious enough bug to deprecate 16.3.x (which is the only way we can warn them). We try to use npm deprecations very sparingly — either for security vulnerabilities or for guaranteed crashes. Otherwise people won't treat them seriously.

I'm sorry that a React bug is affecting your users. One thing you could to is to cut a major release and raise the minimal required React version to 16.4. Then at least it'll be clear to new users.

@Hypnosphi

This comment has been minimized.

Contributor

Hypnosphi commented Aug 24, 2018

Maybe react-polyfill should override 16.3 behavior with current one?

@drcmda

This comment has been minimized.

drcmda commented Sep 6, 2018

@gaearon could i ask for one last thing? Your answer at the top makes it seem like when libs break that's on us, not react, since our code must've been buggy already. Could you please amend this? I'm going to warn users now to upgrade react and i'm going to link to this issue. The new behaviour (which im totally fine with) is definitively changing how react functions, and if any component assumes setState is going through gDSFP it's going to break in 16.3.x, and safeguarding against that will cost some major churn. A warning would be fine, but not when users come back and say "Dan Abramov said its your fault!" 😇

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 6, 2018

Updated to:

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.

I think this should be clearer?

@drcmda

This comment has been minimized.

drcmda commented Sep 6, 2018

nice, thanks! 🙂

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 6, 2018

I'm going to lock this thread since it's super large and everyone gets notified on new comments.
If somebody has further concerns please file a new issue and we'd be happy to discuss them.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.