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

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 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');
Copy link
Member

Choose a reason for hiding this comment

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

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);
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 tweak this to avoid reassignment

@koba04
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
Copy link
Member

ljharb commented Aug 22, 2018

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

@koba04
Copy link
Contributor Author

koba04 commented Aug 22, 2018

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

@ljharb
Copy link
Member

ljharb commented Aug 22, 2018

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

@ljharb
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
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 setState-returning-null-or-undefined-is-noop branch from 6b51df7 to cba495f Compare August 22, 2018 05:26
@ljharb
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.

@Ailrun
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 setState-returning-null-or-undefined-is-noop branch August 22, 2018 05:58
@ljharb ljharb added this to Lifecycles in React 16 Aug 22, 2018
ljharb added a commit that referenced this pull request Aug 25, 2018
 - [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
 - [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
 - [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
Projects
React 16
  
Lifecycles
Development

Successfully merging this pull request may close these issues.

None yet

3 participants