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
Add support for getDerivedStateFromProps #1094
Changes from 3 commits
313c5ed
70886db
a3b5d80
4150a25
78f4418
8726c91
9fbc008
401261a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,206 @@ describe('Lifecycle methods', () => { | |
}); | ||
|
||
|
||
describe('static getDerivedStateFromProps', () => { | ||
it('should set initial state with value returned from getDerivedStateFromProps', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
class Foo extends Component { | ||
static getDerivedStateFromProps(nextProps) { | ||
return { | ||
foo: nextProps.foo, | ||
bar: 'bar' | ||
}; | ||
} | ||
render() { | ||
return <div className={`${this.state.foo} ${this.state.bar}`} />; | ||
} | ||
} | ||
|
||
let element = render(<Foo foo="foo" />, scratch); | ||
expect(element.className).to.be.equal('foo bar'); | ||
}); | ||
|
||
it('should update initial state with value returned from getDerivedStateFromProps', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
class Foo extends Component { | ||
constructor(props, context) { | ||
super(props, context); | ||
this.state = { | ||
foo: 'foo', | ||
bar: 'bar' | ||
}; | ||
} | ||
static getDerivedStateFromProps(nextProps, prevState) { | ||
return { | ||
foo: `not-${prevState.foo}` | ||
}; | ||
} | ||
render() { | ||
return <div className={`${this.state.foo} ${this.state.bar}`} />; | ||
} | ||
} | ||
|
||
let element = render(<Foo />, scratch); | ||
expect(element.className).to.equal('not-foo bar'); | ||
}); | ||
|
||
it('should update the instance\'s state with the value returned from getDerivedStateFromProps when props change', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
class Foo extends Component { | ||
constructor(props, context) { | ||
super(props, context); | ||
this.state = { | ||
value: 'initial' | ||
}; | ||
} | ||
static getDerivedStateFromProps(nextProps) { | ||
if (nextProps.update) { | ||
return { | ||
value: 'updated' | ||
}; | ||
} | ||
|
||
return null; | ||
} | ||
componentDidMount() {} | ||
componentDidUpdate() {} | ||
render() { | ||
return <div className={this.state.value} />; | ||
} | ||
} | ||
|
||
let element; | ||
sinon.spy(Foo, 'getDerivedStateFromProps'); | ||
sinon.spy(Foo.prototype, 'componentDidMount'); | ||
sinon.spy(Foo.prototype, 'componentDidUpdate'); | ||
|
||
element = render(<Foo update={false} />, scratch, element); | ||
expect(element.className).to.equal('initial'); | ||
expect(Foo.getDerivedStateFromProps).to.have.callCount(1); | ||
expect(Foo.prototype.componentDidMount).to.have.callCount(1); // verify mount occurred | ||
expect(Foo.prototype.componentDidUpdate).to.have.callCount(0); | ||
|
||
element = render(<Foo update={true} />, scratch, element); | ||
expect(element.className).to.equal('updated'); | ||
expect(Foo.getDerivedStateFromProps).to.have.callCount(2); | ||
expect(Foo.prototype.componentDidMount).to.have.callCount(1); | ||
expect(Foo.prototype.componentDidUpdate).to.have.callCount(1); // verify update occurred | ||
}); | ||
|
||
it('should NOT be invoked on state only updates', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might change in a minor/patch version. Related: facebook/react#12600 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the heads up! I'll take a look and see what we can do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to handle state-only updates |
||
class Foo extends Component { | ||
constructor(props, context) { | ||
super(props, context); | ||
this.state = { | ||
value: 'initial' | ||
}; | ||
} | ||
static getDerivedStateFromProps(nextProps, prevState) { | ||
// Don't change state for call that happens after the constructor | ||
if (prevState.value === 'initial') { | ||
return null; | ||
} | ||
|
||
return { | ||
value: 'unexpected' | ||
}; | ||
} | ||
componentDidMount() { | ||
this.setState({ value: 'updated' }); | ||
} | ||
render() { | ||
return <div className={this.state.value} />; | ||
} | ||
} | ||
|
||
let element; | ||
sinon.spy(Foo, 'getDerivedStateFromProps'); | ||
|
||
element = render(<Foo />, scratch, element); | ||
expect(element.className).to.equal('initial'); | ||
expect(Foo.getDerivedStateFromProps).to.have.been.calledOnce; | ||
|
||
rerender(); // call rerender to handle cDM setState call | ||
expect(element.className).to.equal('updated'); | ||
expect(Foo.getDerivedStateFromProps).to.have.been.calledOnce; | ||
}); | ||
|
||
it('should NOT modify state if null is returned', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
class Foo extends Component { | ||
constructor(props, context) { | ||
super(props, context); | ||
this.state = { | ||
foo: 'foo', | ||
bar: 'bar' | ||
}; | ||
} | ||
static getDerivedStateFromProps() { | ||
return null; | ||
} | ||
render() { | ||
return <div className={`${this.state.foo} ${this.state.bar}`} />; | ||
} | ||
} | ||
|
||
sinon.spy(Foo, 'getDerivedStateFromProps'); | ||
|
||
let element = render(<Foo />, scratch); | ||
expect(element.className).to.equal('foo bar'); | ||
expect(Foo.getDerivedStateFromProps).to.have.been.called; | ||
}); | ||
|
||
// NOTE: Difference from React | ||
// React v16.3.2 warns if undefined if returned from getDerivedStateFromProps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
it('should NOT modify state if undefined is returned', () => { | ||
class Foo extends Component { | ||
constructor(props, context) { | ||
super(props, context); | ||
this.state = { | ||
foo: 'foo', | ||
bar: 'bar' | ||
}; | ||
} | ||
static getDerivedStateFromProps() {} | ||
render() { | ||
return <div className={`${this.state.foo} ${this.state.bar}`} />; | ||
} | ||
} | ||
|
||
sinon.spy(Foo, 'getDerivedStateFromProps'); | ||
|
||
let element = render(<Foo />, scratch); | ||
expect(element.className).to.equal('foo bar'); | ||
expect(Foo.getDerivedStateFromProps).to.have.been.called; | ||
}); | ||
|
||
// TODO: Consider if componentWillUpdate should still be called | ||
// Likely, componentWillUpdate should not be called only if getSnapshotBeforeUpdate is implemented | ||
it('should NOT invoke deprecated lifecycles (cWM/cWRP) if new static gDSFP is present', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
class Foo extends Component { | ||
static getDerivedStateFromProps() {} | ||
componentWillMount() { | ||
throw new Error('componentWillMount should not be called if getDerivedStateFromProps is present.'); | ||
} | ||
componentWillReceiveProps() { | ||
throw new Error('componentWillReceiveProps should not be called if getDerivedStateFromProps is present.'); | ||
} | ||
render() { | ||
return <div />; | ||
} | ||
} | ||
|
||
sinon.spy(Foo, 'getDerivedStateFromProps'); | ||
sinon.spy(Foo.prototype, 'componentWillMount'); | ||
sinon.spy(Foo.prototype, 'componentWillReceiveProps'); | ||
|
||
render(<Foo />, scratch); | ||
expect(Foo.getDerivedStateFromProps).to.have.been.called; | ||
expect(Foo.prototype.componentWillMount).to.not.have.been.called; | ||
expect(Foo.prototype.componentWillReceiveProps).to.not.have.been.called; | ||
}); | ||
|
||
// TODO: Investigate this test: | ||
// [should not override state with stale values if prevState is spread within getDerivedStateFromProps](https://github.com/facebook/react/blob/25dda90c1ecb0c662ab06e2c80c1ee31e0ae9d36/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js#L1035) | ||
}); | ||
|
||
|
||
describe('#componentWillUpdate', () => { | ||
it('should NOT be called on initial render', () => { | ||
class ReceivePropsComponent extends Component { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we make getDerivedStateFromProps mutually exclusive to componentWillMount and componentWillReceiveProps for two reasons:
It saves 2 bytes! (okay not a real reason, but just thought I'd mention it :) )No longer true after commit 4150a25Thoughts?