Fix to call a setState callback after finishing the the render by koba04 · Pull Request #1453 · enzymejs/enzyme

koba04 · 2017-12-26T15:42:56Z

This PR is to fix #1451.
Currently, a setState callback is called before finishing the render because enzyme is passing the callback into instance.setState.
So I've fixed it to call the callback after this.update()

ljharb · 2017-12-27T03:12:05Z

packages/enzyme/src/ShallowWrapper.js

        this.update();
+        // call the setState callback
+        if (callback) {
+          callback.call(instance);


are we sure that React calls the callback with zero arguments, and the instance as the receiver?

are we sure that React calls the callback with zero arguments, and the instance as the receiver?

I think so.

Implementation

https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberUpdateQueue.js#L352

Demo

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

That implementation doesn't show where update.callback is assigned; but your demo is convincing (for the latest version of React).

However, in React ^15.4, it seems to get one argument set to undefined in your demo (whereas in <= 15.3, and >= 16, it seems to get zero arguments). We should maintain that behavior via adapters.

Thanks! I'll work on to support < 15.3

I've tested for arguments of setState callback.

setState({foo: 'bar'}, function(...args) { console.log(args); });

0.13.3 ... []

0.14.9 ... []

15.4.2 ... [ undefined ]

15.6.2 ... [ undefined ]

16.2.0 ... []

The arguments seem to be [undefined] only in v15.

It's more than just painful; enzyme itself must not know about React at all, given that non-React adapters need to be able to work.

In other words, this functionality needs to be something that every adapter implements, and enzyme merely calls into.

enzyme itself must not know about React at all, given that non-React adapters need to be able to work.

I agree with you. But calling setState callback without argument is depending on current React implementation.
Spying setState callback and using the result later is to reproduce this functionality so I think it can be applied any adapters which are including non-React adapters.

instance.setState(state, spy); : if (callback) { // replay setState callbacks spy.getCalls().forEach((spyCall) => { const context = spyCall.thisValue; const args = spyCall.args; callback.apply(context, args); }); }

Your spy suggestion is interesting, but I was suggesting something like adapter.invokeSetStateCallback(instance, callback) - I'm not sure which is better.

Thanks, I got it.
Are you ok to implement a default behavior into EnzymeAdapter? If not, it will be a breaking change.

Yes, I think that would make sense.

ljharb · 2018-01-05T07:14:36Z

packages/enzyme-test-suite/package.json

    "eslint-plugin-react": "^7.5.1"
  }
-}
+}


let's revert this file; every file should always have a trailing newline

The diff is added again😉 I'm not digging into that but I guess it is added by a script to run tests.

ah, weird. maybe npm itself. not a big deal.

ljharb · 2018-01-05T07:16:14Z

packages/enzyme/src/ShallowWrapper.js

        }
-        instance.setState(state, callback);
+        // We don't pass the setState callback here
+        // to guarantee to call the callback after finishing the render


can we add a test case for this? or does expect(wrapper.find('div').prop('className')).to.eql('bar'); cover that

Yes, expect(wrapper.find('div').prop('className')).to.eql('bar'); is a test for this.

ljharb

LGTM, just some questions

ljharb · 2018-01-05T22:28:20Z

packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx


+
+// some React versions pass undefined as an argument of setState callback.
+const CALLING_SETSTATE_CALLBACK_WITH_UNDEFINED = REACT154 || REACT155 || REACT156;


could this be made a bit more robust by changing it to !REACT15 && !REACT150 && !REACT151 && !REACT152 && !REACT153, since those are the only 4 versions of 15 that will ever not have this behavior?

ljharb · 2018-01-05T22:30:37Z

packages/enzyme-adapter-react-15.4/src/ReactFifteenFourAdapter.js

+    if (MINOR_VERSION >= 4) {
+      callback.call(instance, undefined);
+    } else {
+      callback.call(instance);


maybe this could do super.invokeSetStateCallback(instance, callback)?

ljharb

Thanks!

koba04 · 2018-01-06T01:27:30Z

Thank you for your review!

ljharb · 2018-01-06T03:21:53Z

Tests seem to be failing in React 15, both locally and in travis. I'm going to see what I can do to figure it out now.

koba04 · 2018-01-06T07:10:00Z

packages/enzyme-adapter-react-15.4/src/ReactFifteenFourAdapter.js

+      '^15.4',
+      () => { callback.call(instance, undefined); },
+      () => { super.invokeSetStateCallback(instance, callback); },
+    );


ifReact returns a function, not calling it so you have to call a function which is returned from ifReact.

https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-helper/src/ifReact.js#L7

lol, oops. shouldn't tests have failed?

Will fix in a followup.

I think so we should add a test to guarantee to call a setState callback.
Currently, we don't have a test for it.

koba04 · 2018-01-06T07:20:15Z

packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx

 const BATCHING = !REACT16;

+// some React versions pass undefined as an argument of setState callback.
+const CALLING_SETSTATE_CALLBACK_WITH_UNDEFINED = REACT15 && !REACT150_4;


I guess this condition doesn't cover a case that React v15.4 calls setState callback with undefined.

But all tests are passing in my environment. The reason is ifReact is checking with React v16.2 because enzyme-adapter-react-helper has React v16.2 into its node_modules.
I'm not sure why enzyme-adapter-react-helper has React v16.2 into its node_modules.

it has a peer dep/dev dep on react >= 0.13, so that'd install the latest, but i'm not sure why that would do it. let me try something tho

Thanks! I'll dig into it later.

enzyme-adapter-react-helper shouldn't have react as a devDependencies?

https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-helper/package.json#L50

since it's a peer dep, it also needs to be a dev dep.

Other packages don't have react as a devDeps. Why does only this package have it?

That's a bug leftover from the v3 migration; they all should - every peer dep in every npm package anywhere must always have the identical peer dep string as a dep or a dev dep.

I've almost got a fix PR with a regression test; i'll put it up shortly.

- [fix] actually invoke the setState callback (#1465, #1453) - [fix] add missing support for animation events (#1569) - [fix] call ref for a root element (#1541) - [fix] Allow empty strings as key props (#1524) - [fix] correct the adapter class name - [fix] `mount`: make sure it works with native arrow functions (#1667) - [refactor]: add “lifecycles” adapter option (#1696) - [refactor] use `react-is` package - [deps] remove unneeded `lodash` dep; update `prop-types`, `enzyme-adapter-utils`, `object.assign` - [dev deps] update `babel-cli`, `babel-core`, `babel-plugin-transform-replace-object-assign`, `babel-preset-airbnb`, `chai`, `coveralls`, `eslint`, `eslint-config-airbnb`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`, `lerna`, `json-loader`, `karma-firefox-launcher`, `prop-types`, `lerna`, `rimraf`, `webpack`

Fix to call a setState callback after finishing the the render

04076f6

koba04 mentioned this pull request Dec 26, 2017

setState on shallowWrapper needs explicit update #1451

Closed

ljharb requested changes Dec 27, 2017

View reviewed changes

ljharb added semver: patch API: shallow labels Dec 27, 2017

Implement adapter.invokeSetStateCallback

ba82e1d

koba04 force-pushed the call-setstate-callback-after-update branch from 279cbf6 to d3c6cf1 Compare January 4, 2018 12:17

Add comments for an argument of setState callback

30b4556

ljharb reviewed Jan 5, 2018

View reviewed changes

koba04 force-pushed the call-setstate-callback-after-update branch from 40540e4 to 4afbef7 Compare January 5, 2018 15:10

ljharb reviewed Jan 5, 2018

View reviewed changes

koba04 added 2 commits January 6, 2018 10:06

Make the version check logic more robust

50d7720

Use super.invokeSetStateCallback

d8fa20c

ljharb added semver: minor New stuff. package: react adapter: 15.4 package: react adapter: 15 labels Jan 6, 2018

ljharb approved these changes Jan 6, 2018

View reviewed changes

Fix tests; use ifReact in the 15.4 adapter

751b1f7

ljharb force-pushed the call-setstate-callback-after-update branch 4 times, most recently from 6ea171e to 751b1f7 Compare January 6, 2018 04:57

ljharb merged commit bf14288 into enzymejs:master Jan 6, 2018

koba04 commented Jan 6, 2018

View reviewed changes

ljharb mentioned this pull request Jan 6, 2018

15.4: Fix setState #1465

Merged

This was referenced Oct 1, 2018

Update enzyme in /modules/frontend from 3.3.0 to 3.6.0 project-accelerate/accelerate#265

Open

Update enzyme in / from 3.3.0 to 3.6.0 leonardofaria/leozera-ui#234

Closed



		// some React versions pass undefined as an argument of setState callback.
		const CALLING_SETSTATE_CALLBACK_WITH_UNDEFINED = REACT154 \|\| REACT155 \|\| REACT156;

Uh oh!

Conversation

koba04 commented Dec 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

koba04 commented Jan 6, 2018

Uh oh!

ljharb commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development