Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Write blog post about new async UNSAFE_ and static lifecycles #12047

Closed
bvaughn opened this issue Jan 18, 2018 · 13 comments
Closed

Write blog post about new async UNSAFE_ and static lifecycles #12047

bvaughn opened this issue Jan 18, 2018 · 13 comments
Assignees

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Jan 18, 2018

Follow up item for PR #12028

This blog post should correspond with the 16.3 release. Its primary purpose is to let people know how to prepare for subsequent 16.4 release.

For application developers, this post should explain how to run the codemod to rename the deprecated methods (reactjs/react-codemod/pull/195) as well as general strategies for how to use the new lifecycle(s) instead.

For library maintainers, this should provide recommended update and release strategies including:

  • What kinds of code can be moved into render (to remain backwards compatible) vs what needs to either remain in UNSAFE_componentWillReceiveProps or moved to the new static getDerivedStateFromProps method.
  • When peer dependency version changes are required.
  • What versions of React will support the new UNSAFE_ methods.
@jquense
Copy link
Contributor

jquense commented Mar 19, 2018

Not sure where to stick this, but we've been playing with gDSFP in react-bootstrap and one thing that tripped us up was that you shouldn't just spread through prevState if you have state not derived from props. Otherwise you can drop state updates inadvertantly.

Worth mentioning in the docs (or if it's not intended i can provided a repro)

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 19, 2018

Could you provide a repro? I don't think I understand why spreading prevState would drop updates.

@jquense
Copy link
Contributor

jquense commented Mar 19, 2018

sure :) https://codesandbox.io/s/1y8ryl39vl

@jquense
Copy link
Contributor

jquense commented Mar 19, 2018

(it's not so much that they are dropped as overridden)

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 19, 2018

Cool, thanks for the repro 👍

@gaearon
Copy link
Collaborator

gaearon commented Mar 19, 2018

I think you're saying you'd expect it to work like

  state = { local: 'Not updated', outer: this.props.outer }

  componentWillReceiveProps(nextProps) {
    this.setState(prevState => ({
      ...prevState,
      outer: nextProps.outer
    }));
  }

but it works more like

  state = { local: 'Not updated', outer: this.props.outer }

  componentWillReceiveProps(nextProps) {
    this.setState({
      ...this.state,
      outer: nextProps.outer
    });
  }

?

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 19, 2018

Jason's sandbox looks like a bug to me. I haven't had a chance to dig in yet (in the middle of something) but it's in my queue to look into soonish.

@jquense
Copy link
Contributor

jquense commented Mar 19, 2018

@gaearon yeah, provided its not a bug, it's less intuitive (to me anyway) that prevState could be stale, and I actually thought I had to return prevState. The signature of the method feels more like a straight reducer to state, my assumption was i'd lose the local if it wasn't spread through

@gaearon
Copy link
Collaborator

gaearon commented Mar 19, 2018

@jquense Fair enough—aside from the bug discussion, the part about this not being obvious is unfortunate, but that's already the case for setState API. People were taught to spread there by Redux and can't give up the habit now :-)

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 19, 2018

my assumption was i'd lose the local if it wasn't spread through

This isn't the case- and I'd welcome suggestions to the in-progress docs to make this more clear. (I realize I haven't yet pushed the React.Component reference API page yet though heh.)

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 19, 2018

Looks like this behavior is specific to updates triggered within a React event batched updates.

let childInstance;

class Child extends React.Component {
  state = { local: 0 };
  static getDerivedStateFromProps(nextProps, prevState) {
    return { ...prevState, outer: nextProps.outer };
  }
  updateState = () => {
    this.setState(state => ({ local: state.local + 1 }));
    this.props.onChange(this.state.outer + 1);
  };
  render() {
    childInstance = this;
    return (
      <div onClick={this.updateState}>{`outer:${this.state.outer}, local:${
        this.state.local
      }`}</div>
    );
  }
}

class Parent extends React.Component {
  state = { outer: 0 };
  handleChange = outer => {
    this.setState({ outer });
  };
  render() {
    return <Child outer={this.state.outer} onChange={this.handleChange} />;
  }
}

const instance = ReactTestUtils.renderIntoDocument(<Parent />);
const element = ReactDOM.findDOMNode(instance);
expect(element.textContent).toBe("outer:0, local:0");

// Regular setState
childInstance.updateState();
expect(element.textContent).toBe("outer:1, local:1");

// Batched setState
ReactTestUtils.Simulate.click(element);
expect(element.textContent).toBe("outer:2, local:2");

Only the second update (from the simulated click) demonstrates the unexpected behavior.

@gaearon
Copy link
Collaborator

gaearon commented Mar 20, 2018

The second example doesn't batch updates. Therefore, two setState calls are treated separately and each is synchronous. We don't encounter the issue because the issue is specific to what happens when there are multiple updates in the queue (which there can't be without batching).

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 20, 2018

I'm going to go ahead and close this issue since reactjs/react.dev/pull/596/ and reactjs/react.dev/pull/587 pretty well cover it (even though we can't merge them yet).

@bvaughn bvaughn closed this as completed Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants