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 getSnapshotBeforeUpdate support into shallow #1657

Merged
merged 1 commit into from
Jun 29, 2018

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented May 19, 2018

Fix #1602

This PR is to support getSnapshotBeforeUpdate on shallow.
I've added supportGetSnapshotBeforeUpdate flag into ReactSixteenAdapter to implement this.
As a result, getSnapshotBeforeUpdate is called even if you use react-test-renderer@<16.3.0 so we have to add a new adapter like enzyme-adapter-react16.2 that turn off supportGetSnapshotBeforeUpdate.

You might think getSnapshotBeforeUpdate should be called before render, but the order is same as react-dom.

https://codepen.io/koba04/pen/erbbvB?editors=0011

React calls getSnapshotBeforeUpdate at commit phase and call render at render phase so when getSnapshotBeforeUpdate is called render has already been called.

〜 render phase 〜
Call render

〜 commit phase 〜
Call getSnapshotBeforeUpdate
Update host environment(DOM)
Call componentDidUpdate

So It's reasonable for me that shallow calls getSnapshotBeforeUpdate after render .

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great!

} else {
instance.componentDidUpdate(prevProps, state);
let snapshot;
if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is a bit hard to read - instead of making each step branch on cDU/gSBU - could we have two branches, one that's entirely cDU and one that's entirely gSBU?

@@ -163,6 +163,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
this.options = {
...this.options,
enableComponentDidUpdateOnSetState: true,
supportGetSnapshotBeforeUpdate: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll add a new adapter for 16.2 separately, so this is the proper place to update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed this by #1192

@ljharb ljharb force-pushed the support-getSnapshotBeforeUpdate branch from d9d878b to 3b017f7 Compare June 29, 2018 06:40
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and made the changes.

@ljharb ljharb merged commit 3b017f7 into enzymejs:master Jun 29, 2018
@joshuanutmeg
Copy link

when will this be released? please :)

ljharb added a commit that referenced this pull request Aug 8, 2018
 - [new] add `isFragment` (#1733)
 - [new] Add `displayNameOfNode`, `isValidElementType`
 - [new] `mount`: add `hydrateIn` option (#1317, #1707)
 - [new] Add support for react context element types (#1513)
 - [new] `shallow`: Add getSnapshotBeforeUpdate support (#1657)
 - [fix] portals and roots may render fragments (#1733)
 - [fix] add missing support for animation events (#1569)
 - [fix] `shallow`: SFCs do not get a `this` (#1703)
 - [refactor]: add “lifecycles” adapter option (#1696)
 - [fix] call ref for a root element (#1541)
 - [fix] Allow empty strings as key props (#1524)
 - [fix] Fix ShallowWrapper for array-rendering components (#1498)
 - [refactor] use `react-is` package
 - [meta] ensure a license and readme is present in all packages when published
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
React 16
  
Lifecycles
Development

Successfully merging this pull request may close these issues.

None yet

3 participants