react-test-renderer/shallow setState callback is never called

[benadamstyles]

Do you want to request a feature or report a bug?

Bug, following on from https://github.com/airbnb/enzyme/issues/953

What is the current behavior?

In node_modules/react-test-renderer/cjs/react-test-renderer-shallow.development.js, the callback second argument to setState is never called.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/84v837e9/).

import ShallowRenderer from 'react-test-renderer/shallow'

const renderer = new ShallowRenderer()
renderer.render(<YourComponent />)
const instance = renderer.getMountedInstance()
instance.methodThatCallsSetState()
// => callback passed into setState as the second argument within methodThatCallsSetState is never called

The shallow renderer code is as follows:

Updater.prototype.enqueueSetState = function enqueueSetState(publicInstance, partialState, callback, callerName) {
  if (typeof partialState === 'function') {
    partialState = partialState(publicInstance.state, publicInstance.props);
  }

  this._renderer._newState = _extends({}, publicInstance.state, partialState);

  this._renderer.render(this._renderer._element, this._renderer._context);
};

As you can see, the callback is ignored. In the deep renderer (node_modules/react-test-renderer/cjs/react-test-renderer.development.js), the callback _is_ used:

enqueueSetState: function (instance, partialState, callback) {
  var fiber = ReactInstanceMap_1.get(instance);
  var priorityLevel = getPriorityContext(fiber, false);
  callback = callback === undefined ? null : callback;
  {
    warnOnInvalidCallback(callback, 'setState');
  }
  addUpdate$1(fiber, partialState, callback, priorityLevel);
  scheduleUpdate(fiber, priorityLevel);
}

What is the expected behavior?

The callback is called.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React v16, react-test-renderer v16. I don't know if this worked before.

[gaearon]

Would you be interested in submitting a failing test case as a PR?

cc @bvaughn

[benadamstyles]

Yes absolutely. Will get to it later today, might need some help, I'll ask on here if so!

[benadamstyles]

Hmm ok I have no idea where my test should be. I can't even find the shallow-test-renderer code 😕

[gaearon]

Search for existing tests importing shallow renderer? I bet it's called something like ReactShallowRenderer-test.

[bvaughn]

the callback second argument to setState is never called.

Looks correct, unfortunately. The ReactShallowRendererEntry implementation doesn't use the callback param at all .

@Leeds-eBooks You're looking for ReactTestUtils-test.

Please feel free to assign the PR with this failing test to me. I can review and fix.

[codepodu]

@bvaughn Never contributed to react before. Mind if I take a stab at it? Seems straightforward.

[bvaughn]

Sounds great!

[koba04]

Should the callback bind a public instance of a component to this as other renderers(reconciler) do?

[bvaughn]

Do we do that elsewhere? I think we only do it when we explicitly pass a function references around in such a way as to cause it to lose its context. I'm not very familiar with that code to be honest.

I wouldn't expect the callback to be auto-bound by react.

[codepodu]

Thanks @koba04. I'll update my code.

@bvaughn We do! See https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js#L129 and https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactMount.js#L374

[bvaughn]

I think the ReactMount example is different, because there's no instance to pre-bind to. (It doesn't exist until after you've called render.) In the setState case you have a variety of tools to manage your own bindings (lexical scope, fat-arrow functions, .bind(this)).

I'm not seeing what you describe in ReactUpdateQueue. We aren't binding a this to the callback. We're just queueing up the callback to be called later. (That "later" is part of the code that I'm not very familiar with.)

[koba04]

@bvaughn
My comment was a bit confusing.
What I suggest at the comment is to support the following case.

instance.setState({x: 2}, function() {
  expect(this).toBe(instance);
});

https://github.com/facebook/react/blob/master/src/renderers/__tests__/ReactUpdates-test.js#L243

The callback doesn't bind this, but it is called with function.prototype.call so that this becomes a public instance.

[bvaughn]

That makes sense! 😄

[codepodu]

Yep, I just figured that out, from ReactFiberCommitWork and ReactFiberUpdateQueue

I'll add tests! :)

[codepodu]

@koba04 @bvaughn All done! See #10106