Add support for getDerivedStateFromProps #1094

andrewiggins · 2018-05-08T05:35:14Z

Add support for the getDerivedStateFromProps lifecycle method.

Where componentWillMount or componentWillReceiveProps would've previously been called, Preact now invokes getDerivedStateFromProps on the component's constructor. The result of getDerivedStateFromProps is then immediately merged into the component's current state for use by the next lifecycle methods (primarily shouldComponentUpdate, render, and componentDidUpdate).

Sizes (bytes gzipped)
Before: 3443

~~After: 3477~~
~~Change: +34~~

commit 4150a25
After: 3480
Change: +37

Some differences from React

React warns if getDerivedStateFromProps is defined on Functional Components (source).
React warns if getDerivedStateFromProps is defined but state is not initialized (source). Preact always initializes state to an empty object (source) so this isn't necessary.
React warns if getDerivedStateFromProps returns undefined (source). This PR allows undefined to be returned.

See comments for more discussion.

Updated sizes.

andrewiggins · 2018-05-08T06:11:03Z

src/vdom/component.js

-	if (!component.base || mountAll) {
+	if (component.constructor.getDerivedStateFromProps) {
+		component.state = extend(component.state, component.constructor.getDerivedStateFromProps(props, component.state));
+	} else if (!component.base || mountAll) {


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 4150a25

React makes them mutually exclusive. And while Preact can differ from React, I think in this instance it might be better if follow their lead. Matching React's behavior prevents us from having to define, document, and maintain the interaction between getDerivedStateFromProps and the deprecated lifecycle methods.

Thoughts?

andrewiggins · 2018-05-08T16:01:38Z

test/browser/lifecycle.js

@@ -23,6 +23,206 @@ describe('Lifecycle methods', () => {
 	});


+	describe('static getDerivedStateFromProps', () => {
+		it('should set initial state with value returned from getDerivedStateFromProps', () => {


Corresponding React unit test from commit 25dda90c

andrewiggins · 2018-05-08T16:04:36Z

test/browser/lifecycle.js

+			expect(element.className).to.be.equal('foo bar');
+		});
+
+		it('should update initial state with value returned from getDerivedStateFromProps', () => {


Corresponding React unit test from commit 25dda90c

andrewiggins · 2018-05-08T16:05:40Z

test/browser/lifecycle.js

+			expect(element.className).to.equal('not-foo bar');
+		});
+
+		it('should update the instance\'s state with the value returned from getDerivedStateFromProps when props change', () => {


Corresponding React unit test from commit 25dda90c

andrewiggins · 2018-05-08T16:07:27Z

test/browser/lifecycle.js

+			expect(Foo.prototype.componentDidUpdate).to.have.callCount(1); // verify update occurred
+		});
+
+		it('should NOT be invoked on state only updates', () => {


Codesandbox demonstrating this behavior in React v16.3.2

This might change in a minor/patch version. Related: facebook/react#12600 (comment).

Thanks for the heads up! I'll take a look and see what we can do

Updated to handle state-only updates

andrewiggins · 2018-05-08T16:11:08Z

test/browser/lifecycle.js

+			expect(Foo.getDerivedStateFromProps).to.have.been.calledOnce;
+		});
+
+		it('should NOT modify state if null is returned', () => {


React doc defining behavior for null

andrewiggins · 2018-05-08T16:12:34Z

test/browser/lifecycle.js

+		});
+
+		// NOTE: Difference from React
+		// React v16.3.2 warns if undefined if returned from getDerivedStateFromProps


Corresponding React unit test from commit 25dda90c demonstrating warning behavior for undefined return values

andrewiggins · 2018-05-08T16:13:55Z

test/browser/lifecycle.js

+
+		// 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', () => {


Corresponding React unit test from commit 25dda90c

coveralls · 2018-05-08T16:34:40Z

Coverage remained the same at 100.0% when pulling 401261a on andrewiggins:getDerivedStateFromProps-2 into 4f57ba0 on developit:master.

marvinhagemeister · 2018-05-13T19:52:30Z

@andrewiggins Thanks for making a PR for this 🎉 I have to read a bit more on the backstory of getDerivedStateFromProps to be able to review it. From a quick glance your implementation looks great! Love that you compared notes with the react tests and tried to make the changes as small as possible.

I'm currently lying in bed with an illness, but will review this once my health is back up again 🎉

andrewiggins · 2018-05-15T16:19:09Z

@marvinhagemeister Hope you are feeling a little better! And no worries. I appreciate you circling back. I'm anticipating there will be more discussion and planning around what to do and how to deliver new lifecycle methods before we ship this.

developit

Looks good. Let's see if we can nudge that size at all, otherwise I am happy with this. Thanks for your hard work!

developit · 2018-05-16T20:15:17Z

src/vdom/component.js

-	}
-	else if (component.componentWillReceiveProps) {
-		component.componentWillReceiveProps(props, context);
+	if (typeof component.constructor.getDerivedStateFromProps === 'undefined') {


I'd be open to loosening this check if it can help offset the size.

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.

@andrewiggins I think he meant removing the if-statement completely.

Oh, whoops. That makes sense. Yea, that would save about 4 bytes I believe.

developit · 2018-05-16T20:16:13Z

src/vdom/component.js

@@ -73,6 +75,11 @@ export function renderComponent(component, opts, mountAll, isChild) {
 		skip = false,
 		rendered, inst, cbase;

+	let getDerivedStateFromProps = component.constructor.getDerivedStateFromProps;


It's usually smaller post-gzip to skip an intermediary variable like this one. Any chance that's the case here?

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.

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.

marvinhagemeister

LGTM 🎉 @andrewiggins I'm really in awe of the thoroughness of this PR. You really went above all expectations by adding links to reacts unit tests and including them in our own suite.

Really solid and polished PR, thanks again for your contribution 🎉

marvinhagemeister · 2018-05-19T09:37:53Z

src/vdom/component.js

-	}
-	else if (component.componentWillReceiveProps) {
-		component.componentWillReceiveProps(props, context);
+	if (typeof component.constructor.getDerivedStateFromProps === 'undefined') {


@andrewiggins I think he meant removing the if-statement completely.

andrewiggins · 2018-05-21T03:26:47Z

@marvinhagemeister Thanks for kind words 😄. I really appreciate the hard work @developit, you, and the other Preact contributors have put into building a high quality library in so few bytes. I want/hope my PRs meet that same bar.

andrewiggins added 2 commits May 7, 2018 19:46

Add getDerivedStateFromProps implementation

313c5ed

Add getDerivedStateFromProps unit tests

70886db

andrewiggins commented May 8, 2018

View reviewed changes

Fix UT linting errors

a3b5d80

andrewiggins mentioned this pull request May 8, 2018

Decide what to do with upcoming react api changes #1047

Closed

9 tasks

Call getDerivedStateFromProps on state-only updates

4150a25

Merge branch 'master' into getDerivedStateFromProps-2

78f4418

developit reviewed May 16, 2018

View reviewed changes

Merge branch 'master' into getDerivedStateFromProps-2

8726c91

developit assigned developit and unassigned developit May 16, 2018

Merge branch 'master' into getDerivedStateFromProps-2

9fbc008

andrewiggins force-pushed the getDerivedStateFromProps-2 branch from f895912 to 7a905ae Compare May 17, 2018 05:29

Remove intermediate gDSFP variable

401261a

andrewiggins force-pushed the getDerivedStateFromProps-2 branch from 7a905ae to 401261a Compare May 17, 2018 05:34

marvinhagemeister approved these changes May 19, 2018

View reviewed changes

marvinhagemeister merged commit b4d7aee into preactjs:master May 19, 2018

andrewiggins deleted the getDerivedStateFromProps-2 branch May 21, 2018 03:16

valotas mentioned this pull request May 21, 2018

WIP: getDerivedStateFromProps #1114

Closed

This was referenced Aug 16, 2018

The renderToString function does not call component's static getDerivedStateFromProps preactjs/preact-render-to-string#62

Closed

Adds support for getDerivedStateFromProps preactjs/preact-render-to-string#63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for getDerivedStateFromProps #1094

Add support for getDerivedStateFromProps #1094

andrewiggins commented May 8, 2018 •

edited

andrewiggins May 8, 2018 •

edited

andrewiggins May 8, 2018 •

edited

andrewiggins May 8, 2018

andrewiggins May 8, 2018

andrewiggins May 8, 2018

thysultan May 8, 2018

andrewiggins May 8, 2018 •

edited

andrewiggins May 8, 2018

andrewiggins May 8, 2018

andrewiggins May 8, 2018

andrewiggins May 8, 2018

coveralls commented May 8, 2018 •

edited

marvinhagemeister commented May 13, 2018

andrewiggins commented May 15, 2018

developit left a comment

developit May 16, 2018

andrewiggins May 17, 2018

marvinhagemeister May 19, 2018

andrewiggins May 21, 2018

developit May 16, 2018

andrewiggins May 17, 2018 •

edited

andrewiggins May 17, 2018 •

edited

marvinhagemeister left a comment

marvinhagemeister May 19, 2018

andrewiggins commented May 21, 2018

Add support for getDerivedStateFromProps #1094

Add support for getDerivedStateFromProps #1094

Conversation

andrewiggins commented May 8, 2018 • edited

andrewiggins May 8, 2018 • edited

Choose a reason for hiding this comment

andrewiggins May 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewiggins May 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 8, 2018 • edited

marvinhagemeister commented May 13, 2018

andrewiggins commented May 15, 2018

developit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewiggins May 17, 2018 • edited

Choose a reason for hiding this comment

andrewiggins May 17, 2018 • edited

Choose a reason for hiding this comment

marvinhagemeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewiggins commented May 21, 2018

andrewiggins commented May 8, 2018 •

edited

andrewiggins May 8, 2018 •

edited

andrewiggins May 8, 2018 •

edited

andrewiggins May 8, 2018 •

edited

coveralls commented May 8, 2018 •

edited

andrewiggins May 17, 2018 •

edited

andrewiggins May 17, 2018 •

edited