Add getSnapshotBeforeUpdate lifecycle hook #1112
Conversation
marvinhagemeister
commented
May 20, 2018
•
marvinhagemeister
commented
- Add getSnapshotBeforeUpdate tests
- Port unit tests from react
- Add unit tests
test/browser/lifecycle.js
Outdated
| @@ -218,6 +218,181 @@ describe('Lifecycle methods', () => { | |||
| // [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.only("#getSnapshotBeforeUpdate", () => { | |||
There was a problem hiding this comment.
TODO: remove only
src/vdom/component.js
Outdated
| rendered, inst, cbase; | ||
|
|
||
| // FIXME: Do we have an issue with previous state?? | ||
| const prev = extend({}, previousState); |
There was a problem hiding this comment.
Dirty workaround: I have the suspicion that previousState is mutated somewhere. Caught this with the tests for getSnapshotBeforeUpdate by react.
TODO: Check what's happening here. Seems fishy and even more worrying is that we don't seem to have a test for that yet :S
marvinhagemeister
changed the title
src/vdom/component.js
Outdated
| @@ -68,13 +68,14 @@ export function renderComponent(component, opts, mountAll, isChild) { | |||
| state = component.state, | |||
| context = component.context, | |||
| previousProps = component.prevProps || props, | |||
| previousState = component.prevState || state, | |||
| previousState = extend({}, component.prevState || state), | |||
There was a problem hiding this comment.
Although this is needed for getSnapshotBeforeUpdate this also fixes a bug with componentDidUpdate. During rendering, the state variable is mutated. This has the consequence, that previousState would never hold the previous state, but the most current one.
There was a problem hiding this comment.
where were we mutating previousState?
There was a problem hiding this comment.
Ah, should have debugged this further. The mutation happens when getDerivedStateFromProps is used. Moved the extend call there, so that we limit the cloning and only do it when necessary.
|
This PR is now ready to review 🎉 |
src/vdom/component.js
Outdated
| previousContext = component.prevContext || context, | ||
| isUpdate = component.base, | ||
| nextBase = component.nextBase, | ||
| initialBase = isUpdate || nextBase, | ||
| initialChildComponent = component._component, | ||
| skip = false, | ||
| snapshot = previousContext, |
There was a problem hiding this comment.
Q: were we deviating from React here by passing context as a 3rd argument to componentDidUpdate? If so, maybe we correct that here.
There was a problem hiding this comment.
Yep, we were/are deviating from React there. React never had a third context parameter. Same for shouldComponentUpdate. I'd prefer to make these changes in a separate PR, because they are independent from implementing getSnapshotBeforeUpdate.
There was a problem hiding this comment.
Agreed regarding the separate PR, but we should make sure they go out in the same release - we'll save a few bytes by dropping that reassignment.
There was a problem hiding this comment.
mh I have a branch where I removed the non-standard context arguments, but I get the exact same gzip size as in master :S
test/browser/lifecycle.js
Outdated
| @@ -218,6 +218,181 @@ describe('Lifecycle methods', () => { | |||
| // [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("#getSnapshotBeforeUpdate", () => { | |||
| it('should call nested new lifecycle methods in the right order', () => { | |||
There was a problem hiding this comment.
Love this test! This might be out of scope for this PR, but could we move this test to be directly under the "Lifecycle methods" describe block or put it in its own describe block? This test specificies the behavior of more than just getSnapshotBeforeUpdate. It specifies the order of (potentially all) lifecycle methods. I think it would be valuable to expand this test to include the order of the lifecycle methods for all 4 main render "events": mounting, props update, state update, and unmounting.
I started a test similar to this when implementing getDerivedStateFromProps but pulled it out as it was getting messy. This implementation is much more succinct. I'd be happy to expand this test in another PR if you think its useful.
There was a problem hiding this comment.
That's a great suggestion! Moved the test to the top describe-block 🎉
I'm a bit hesitant to add componentWillUpdate, componentWillMount and componentWillReceiveProps to the test, because we'll follow react and mark them as deprecated with our next release.
There was a problem hiding this comment.
Yea, that makes total sense. No need to add them
| rendered, inst, cbase; | ||
|
|
||
| if (component.constructor.getDerivedStateFromProps) { | ||
| state = component.state = extend(state, component.constructor.getDerivedStateFromProps(props, state)); | ||
| previousState = extend({}, previousState); |
test/browser/lifecycle.js
Outdated
| ]); | ||
| }); | ||
|
|
||
| it('should pass the return value from getSnapshotBeforeUpdate to componentDidUpdate', () => { |
There was a problem hiding this comment.
I appreciate the mirroring of the React UT suite 😄
