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

Fix to skip updates when nextState is null or undefined #1785

Merged

Conversation

@koba04
Copy link
Contributor

koba04 commented Aug 22, 2018

Fix #1783
This PR makes to prevent updates when setState returns null or undefined.
But the callback of setState's 2nd arguments should always be called as well as a behavior of ReactDOM.

@ljharb
Copy link
Member

ljharb left a comment

Thanks, this looks great! Just two small tweaks (I'm happy to take care of it before merging)

}

const wrapper = mount(<Foo />);
const spy = sinon.spy(wrapper.instance(), 'componentDidUpdate');

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 22, 2018

Member

i think we can check both null and undefined here by spying on componentDidUpdate?

@@ -446,6 +447,14 @@ class ShallowWrapper {
const prevProps = instance.props;
const prevState = instance.state;
const prevContext = instance.context;

if (typeof state === 'function') {
state = state.call(instance, prevState, prevProps);

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 22, 2018

Member

i'll tweak this to avoid reassignment

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Aug 22, 2018

Travis is failed.
Ah, This behavior seems to be >= 16, so we need an adapter flag for this.

@ljharb ljharb force-pushed the koba04:setState-returning-null-or-undefined-is-noop branch from b048ce3 to 6b51df7 Aug 22, 2018

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Aug 22, 2018

@koba04 meaning, prior to react 16, it's an infinite loop?

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Aug 22, 2018

@ljharb Yeah, prior to react 16, This seems to cause an infinite loop.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Aug 22, 2018

Sounds good, I'll finish this one up real quick.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Aug 22, 2018

(I don't think an adapter flag is needed; the test just needs to run only in react 16+)

@koba04

This comment has been minimized.

Copy link
Contributor Author

koba04 commented Aug 22, 2018

It might be relevant to the test, not the patch itself.

@ljharb ljharb force-pushed the koba04:setState-returning-null-or-undefined-is-noop branch from 6b51df7 to cba495f Aug 22, 2018

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Aug 22, 2018

actually i changed my mind; this should be in the adapter, so that react versions < 16 always get the infinite loop, even shallow-rendering when setState returns nullish.

@ljharb

ljharb approved these changes Aug 22, 2018

@ljharb ljharb merged commit 5a4bb4e into airbnb:master Aug 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Ailrun

This comment has been minimized.

Copy link

Ailrun commented Aug 22, 2018

I missed so many details in my issue... 😞 Thank you for noticing the version issue and fixing it too! You guys are really wonderfully awesome!

@koba04 koba04 deleted the koba04:setState-returning-null-or-undefined-is-noop branch Aug 22, 2018

@ljharb ljharb added this to Lifecycles in React 16 Aug 22, 2018

ljharb added a commit that referenced this pull request Aug 25, 2018

[enzyme-adapter-react-{16.1,16.2,16.3}] v1.1.0
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - [New] pass the adapter into `createMountWrapper` (#1592, @jquense)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - update deps
 - [meta] ensure a license and readme is present in all packages when published

ljharb added a commit that referenced this pull request Aug 25, 2018

[enzyme-adapter-react-16] v1.3.0
 - [New] Add forwardRef support (#1592, @jquense)
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [New] Add `displayNameOfNode`, `isValidElementType` (#1701, @jquense)
 - [New] pass the adapter into `createMountWrapper` (#1592, @jquense)
 - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - update deps
 - [meta] ensure a license and readme is present in all packages when published

ljharb added a commit that referenced this pull request Aug 25, 2018

[enzyme] v3.5.0
 - [New] Add forwardRef support (#1592, @jquense)
 - [New] Add Portal support (#1760, #1761, #1772, #1774, @jgzuke)
 - [New] Add pointer events support (#1753, @ljharb)
 - [Fix] preemptively fix compat with React v16.4.3 (#1790, #1778, @gaearon, @aweary)
 - [Fix] `shallow`: prevent rerenders with PureComponents (#1786, @koba04)
 - [Fix] `shallow`: skip updates when nextState is `null` or `undefined` (#1785, @koba04)
 - [Fix] `shallow`: `setState` after `setProps` calls `componentWillReceiveProps` (#1779, @peanutenthusiast)
 - [Fix] `mount`/`shallow`: be stricter on the wrapper’s setState/setProps callback
 - [Fix] `shallow`/`mount`: improve error message when wrapping invalid elements (#1759, @jgzuke)
 - update deps
 - [Refactor] remove most uses of lodash
 - [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
You can’t perform that action at this time.