Fixes #509: pass callback on setState and setProps #617

alecrobins · 2016-09-30T16:03:40Z

Resolves #509

Due to the asynchronous nature of setState, we can't assume a wrapper.setState or wrapper.setProps (which uses setState in the setChildProps call) will act synchronously.

This PR adds callback functions to the stateState and setProps functions for full DOM rendering and setState for shallow rendering.

Example:

class Foo extends React.Component {
  constructor(props) {
    super(props);
    this.state = { id: 'foo' };
  }
  render() {
    return (
      <div className={this.state.id} />
    );
  }
}
const wrapper = mount(<Foo />);
expect(wrapper.state()).to.eql({ id: 'foo' });
wrapper.setState({ id: 'bar' }, () => {
  expect(wrapper.state()).to.eql({ id: 'bar' });
});

ljharb · 2016-09-30T17:13:27Z

src/ReactWrapper.jsx

    if (this.root !== this) {
      throw new Error('ReactWrapper::setState() can only be called on the root');
    }
-    this.instance().setState(state);
+    this.instance().setState(state, cb);


does react handle throwing if cb isn't a function?

Yea seems like it's taken care of here:
https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js#L257

Awesome - it would still be good to add tests for that behavior, in case a future version of react changes it

Yea sounds good. I'll add a test.

ljharb · 2016-09-30T17:14:00Z

src/ReactWrapper.jsx

   * @returns {ReactWrapper}
   */
-  setState(state) {
+  setState(state, cb) {


since this param is optional, it should default to undefined so it doesn't count towards the function's length (same throughout)

ljharb · 2016-09-30T17:25:01Z

test/ReactWrapper-spec.jsx

+      expect(wrapper.find('.foo').length).to.equal(1);
+      wrapper.setProps({ id: 'bar', foo: 'bla' }, () => {
+        expect(wrapper.find('.bar').length).to.equal(1);
+      });


is there a way we could add a sync assertion that the length is NOT equal to 1, to prove that the callback is doing its job?

It is difficult to force setState to act asynchronously. In most cases it does act synchronously it's just not a guarantee.

hm - the motivation for adding this callback is those cases where it does act async. there must be some likely usage pattern that forces it?

I'm not sure usage patterns that setState does act async in testing. but I think you can use batchedUpdates like this.

import { batchedUpdates } from '../src/react-compat'; batchedUpdates(() => { wrapper.setProps({ id: 'bar', foo: 'bla' }, () => { expect(wrapper.find('.bar').length).to.equal(1); }); expect(wrapper.find('.bar').length).to.equal(0); });

thanks @koba04. Using batchedUpdates worked.

aweary · 2016-10-01T01:03:52Z

src/ReactWrapperComponent.jsx

@@ -32,9 +32,9 @@ export default function createWrapperComponent(node, options = {}) {
      };
    },

-    setChildProps(newProps) {
+    setChildProps(newProps, cb = undefined) {


Wont cb be undefined by default if its not passed in?

Yes, but defaulting it explicitly makes it not count towards the function's length.

What are the advantages of doing that here? Specifically for setState which is meant to shadow the React API, which does not make this explicit.

Function lengths are intended to only count required arguments - the language follows this for the most part. You're right that React doesn't make this explicit, but I'd prefer to correct their mistake for our API.

aronwoost · 2016-10-07T16:55:29Z

Awesome work! Well, should it be shipped?

ljharb

This LGTM - it would still be preferred, imo, to have a test that guarantees that the setState callback is async (we don't have that right now) - but I don't think that needs to block.

alecrobins · 2016-10-07T18:10:36Z

@ljharb I added a test where the callback is called asynchronously ( 7b94db8#diff-2f849fa739461853c70ddd193891074fR976 )

ljharb · 2016-10-07T18:12:31Z

@alecrobins awesome, thanks! Would you mind rebasing so as to remove all merge commits, and then this is good to go.

lencioni · 2016-10-11T16:18:01Z

@alecrobins can you rebase this again for us when you have a moment? Thanks!

lencioni · 2016-10-11T16:18:50Z

docs/README.md

@@ -57,7 +57,7 @@
    * [render()](/docs/api/ShallowWrapper/render.md)
    * [setContext(context)](/docs/api/ShallowWrapper/setContext.md)
    * [setProps(nextProps)](/docs/api/ShallowWrapper/setProps.md)
-    * [setState(nextState)](/docs/api/ShallowWrapper/setState.md)
+    * [setState(nextState[, cb])](/docs/api/ShallowWrapper/setState.md)


It would be nice to use callback for this variable (here and elsewhere), for maximum clarity.

alecrobins · 2016-10-11T18:14:45Z

@lencioni Just rebased and changed the variable name to callback.

lencioni · 2016-10-11T21:22:47Z

LGTM

kentor · 2016-11-16T19:27:33Z

hi! when will this get released?

Looking forward to do this in tests:

flushSetState = (wrapper) => new Promise(resolve => wrapper.setState({}, resolve));
...
await flushSetState(wrapper);
expect(wrapper.state(...))

koba04 · 2016-11-17T01:14:48Z

@kentor This has been released in 2.5.0.

https://github.com/airbnb/enzyme/blob/master/CHANGELOG.md#250-october-17-2016

ljharb requested changes Sep 30, 2016

View reviewed changes

ljharb added API: shallow API: mount semver: minor New stuff. labels Sep 30, 2016

aweary reviewed Oct 1, 2016

View reviewed changes

alecrobins force-pushed the fix-509 branch 2 times, most recently from 4ed9c95 to 7b94db8 Compare October 3, 2016 14:37

ljharb approved these changes Oct 7, 2016

View reviewed changes

alecrobins force-pushed the fix-509 branch from 0331b47 to 7ec03a0 Compare October 7, 2016 19:04

lencioni reviewed Oct 11, 2016

View reviewed changes

alecrobins and others added 7 commits October 11, 2016 13:11

Update docs

ef799ce

Add callback to setProps and setState

96aaea6

Add tests

260370a

Default cb to undefined so it doesn't count toward functions length

3281333

Add test for cb error if not function

faef47c

Use batchedUpdates for setProps test

7ed37aa

Use callback instead of cb

2183d65

alecrobins force-pushed the fix-509 branch from 96eab35 to 2183d65 Compare October 11, 2016 17:11

lencioni approved these changes Oct 11, 2016

View reviewed changes

lencioni merged commit c1e03cc into enzymejs:master Oct 11, 2016

willdurand mentioned this pull request Sep 5, 2017

ShowMoreCard should expand if updated content changes mozilla/addons-frontend#3057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #509: pass callback on setState and setProps #617

Fixes #509: pass callback on setState and setProps #617

alecrobins commented Sep 30, 2016

ljharb Sep 30, 2016

alecrobins Sep 30, 2016

ljharb Sep 30, 2016

alecrobins Sep 30, 2016

ljharb Sep 30, 2016

ljharb Sep 30, 2016

alecrobins Sep 30, 2016

ljharb Sep 30, 2016

koba04 Oct 1, 2016

alecrobins Oct 2, 2016

aweary Oct 1, 2016

ljharb Oct 1, 2016

aweary Oct 1, 2016

ljharb Oct 1, 2016

aronwoost commented Oct 7, 2016

ljharb left a comment

alecrobins commented Oct 7, 2016

ljharb commented Oct 7, 2016

lencioni commented Oct 11, 2016

lencioni Oct 11, 2016 •

edited

Loading

alecrobins commented Oct 11, 2016

lencioni commented Oct 11, 2016

kentor commented Nov 16, 2016 •

edited

Loading

koba04 commented Nov 17, 2016

Fixes #509: pass callback on setState and setProps #617

Fixes #509: pass callback on setState and setProps #617

Conversation

alecrobins commented Sep 30, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aronwoost commented Oct 7, 2016

ljharb left a comment

Choose a reason for hiding this comment

alecrobins commented Oct 7, 2016

ljharb commented Oct 7, 2016

lencioni commented Oct 11, 2016

lencioni Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

alecrobins commented Oct 11, 2016

lencioni commented Oct 11, 2016

kentor commented Nov 16, 2016 • edited Loading

koba04 commented Nov 17, 2016

lencioni Oct 11, 2016 •

edited

Loading

kentor commented Nov 16, 2016 •

edited

Loading