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

Add support for getDerivedStateFromProps #1094

Merged
17 changes: 12 additions & 5 deletions src/vdom/component.js
Expand Up @@ -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') {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

if (!component.base || mountAll) {
if (component.componentWillMount) component.componentWillMount();
}
else if (component.componentWillReceiveProps) {
component.componentWillReceiveProps(props, context);
}
}

if (context && context!==component.context) {
Expand Down Expand Up @@ -73,6 +75,11 @@ export function renderComponent(component, opts, mountAll, isChild) {
skip = false,
rendered, inst, cbase;

let getDerivedStateFromProps = component.constructor.getDerivedStateFromProps;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
Expand Down
196 changes: 196 additions & 0 deletions test/browser/lifecycle.js
Expand Up @@ -23,6 +23,202 @@ describe('Lifecycle methods', () => {
});


describe('static getDerivedStateFromProps', () => {
it('should set initial state with value returned from getDerivedStateFromProps', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member Author

Choose a reason for hiding this comment

The 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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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 {
Expand Down