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 5 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 |
---|---|---|
|
@@ -20,11 +20,13 @@ export function setComponentProps(component, props, opts, context, mountAll) { | |
if ((component.__ref = props.ref)) delete props.ref; | ||
if ((component.__key = props.key)) delete props.key; | ||
|
||
if (!component.base || mountAll) { | ||
if (component.componentWillMount) component.componentWillMount(); | ||
} | ||
else if (component.componentWillReceiveProps) { | ||
component.componentWillReceiveProps(props, context); | ||
if (typeof component.constructor.getDerivedStateFromProps === 'undefined') { | ||
if (!component.base || mountAll) { | ||
if (component.componentWillMount) component.componentWillMount(); | ||
} | ||
else if (component.componentWillReceiveProps) { | ||
component.componentWillReceiveProps(props, context); | ||
} | ||
} | ||
|
||
if (context && context!==component.context) { | ||
|
@@ -73,6 +75,11 @@ export function renderComponent(component, opts, mountAll, isChild) { | |
skip = false, | ||
rendered, inst, cbase; | ||
|
||
let getDerivedStateFromProps = component.constructor.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's usually smaller post-gzip to skip an intermediary variable like this one. Any chance that's the case here? 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. Interesting, in this case it appears to not move the byte count at all. 3480 before and 3480 after the change (you can see it in commit b165370). So which do you prefer? Intermediary variable or not? Happy to update it to which ever. 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. Spoke too soon. When this change is combined with other changes, it could save an additional byte. I've updated the code to remove the intermediary variable. |
||
if (getDerivedStateFromProps) { | ||
state = component.state = extend(state, getDerivedStateFromProps(props, state)); | ||
} | ||
|
||
// if updating | ||
if (isUpdate) { | ||
component.props = previousProps; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,202 @@ 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 update the instance\'s state with the value returned from getDerivedStateFromProps when state changes', () => { | ||
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: prevState.value + ' derived' | ||
}; | ||
} | ||
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 derived'); | ||
expect(Foo.getDerivedStateFromProps).to.have.been.calledTwice; | ||
}); | ||
|
||
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() {} | ||
componentWillReceiveProps() {} | ||
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'd be open to loosening this check if it can help offset the size.
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.
By loosening do you mean making the check less specific (i.e
if (!component.constructor.getDerivedStateFromProps)
)? I just tried that change in commit 3f51408 and it actually increased the size by 2 bytes.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.
@andrewiggins I think he meant removing the
if
-statement completely.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.
Oh, whoops. That makes sense. Yea, that would save about 4 bytes I believe.