change the argument passed to CallbackQueue.getPooled

[gwmccull]

fix to #10100

[gaearon]

This code has not changed in a very long time, and doesn't seem to have changed between 15 and 16.

Could you please:

1. Add a unit test for the regression
2. Explain why this wasn't a problem in 15 despite the same code at this line

Thanks!

[gwmccull]

@gaearon ok, I think I mostly have it worked out. The code change that actually exposed this bug was in:

/src/renderers/shared/stack/reconciler/ReactUpdates.js

In version 15.4.1, you see this code:

var UPDATE_QUEUEING = {
  initialize: function() {
    this.callbackQueue.reset();
  },
  close: function() {
    this.callbackQueue.notifyAll();
  },
};

function ReactUpdatesFlushTransaction() {
  /* redacted */
  this.callbackQueue = CallbackQueue.getPooled();
  /* redacted */
}

The setState callbacks are getting pushed into that callbackQueue and run in that context. I can prove it by adding changing it to CallbackQueue.getPooled(1) and seeing that my callback function is run with a parameter of 1.

Since getPooled has no arguments, the parameter passed to the callback function will always be undefined.

In version 16, all of that code is gone and the CallbackQueue is apparently being taken from the component or another intermediary instead. That's why when I made the change in my PR, it changed the callback function from being called with null to being called with undefined.

I've created a unit test for ReactNativeReconcileTransaction but this is kind of a hard issue to test since testing the bug I actually experienced would involve unit tests on the React Native side of things.

[gaearon]

I've created a unit test for ReactNativeReconcileTransaction but this is kind of a hard issue to test since testing the bug I actually experienced would involve unit tests on the React Native side of things.

Can you rewrite the test to verify observable API instead of internals? In this particular case, you could write a test that schedules a setState callback, and then assert that it does’t get extra arguments.

This is important because the "transaction" code will be removed soon anyway. We want to ensure we doesn't regress on this behavior even if it's implemented in some different way.

To force it to follow RN code path, you can do something like this.

[gwmccull]

I replaced the test with one that tests setState directly with a mounted component.

the format of the test is a bit odd. Originally, I had:

  it('calls setState with the correct parameters', () => {
    var mockCallback = jest.fn();
    class Component extends React.Component {
      componentDidMount() {
        this.setState({}, mockCallback);
      }
      render() {
        return false;
      }
    }

    ReactNative.render(<Component />, 11);
    expect(mockCallback).toBeCalledWith(undefined);
  });

That test worked as expected when I ran jest on it directly. But when I ran the scripts/fiber/record-tests it failed. Somehow the mock function was being called with [undefined] when I ran the test directly but [] when I ran the test with the script.

I'm not sure what is causing the difference but this gets at the arguments with a closure rather than the jest mock functions so it should be a valid test

[gaearon]

Can you please run yarn prettier to prettify the code and commit the result?
Thanks!

[gaearon]

Let's add a similar test to ReactTestUtils-test for shallow renderer, and to ReactCompositeComponent-test for DOM renderer.

[gwmccull]

done

[gwmccull]

@gaearon I created those additional tests but to get them to pass, I had to fix another instance of that bug. It looks like that code is related to React for the web.

I don't use React on the web so I can't easily verify if this was an issue

[gwmccull]

I finally got prettier to run.

npm run prettier didn't seem to do anything. Had to run npm run prettier-all

[gwmccull]

@gaearon Do I need to do anything else on this PR? Just want to make sure this isn't waiting on me to do something

[gaearon]

Small changes.

[gaearon]

Let's change this to should call setState callback with no arguments (and others too)

[gaearon]

Maybe let's assert arguments.length is 0 instead of checking first argument?

[gwmccull]

so I tried this but npm test causes the length of arguments to be 1, [ undefined ]. But node ./scripts/fiber/record-tests has the length of the arguments as 0, [ ]

So it seems like checking the first argument is the only way to go that I see

[gaearon]

Can you see where that undefined comes from? I’d like to understand it.

[gaearon]

Thanks for your work. This mostly looks good, just a few final comments.

[gaearon]

The extra argument is coming from here. Seems like CallbackQueue assumes argument always exists. (cc @aweary, we introduced this arity issue in #7649)

But I don’t see us using this argument anymore. If you search for getPooled() calls it’s always null. The code that relied on it was deleted.

So let’s just completely remove _arg from CallbackQueue, stop passing it, and remove the argument to getPooled() as well. Then you can fix up the tests to check arguments.length too.

[gwmccull]

@gaearon ok, I removed arg and cleaned up the Flow types for CallbackQueue. I also fixed up the tests

[gaearon]

This looks awesome. Thank you! This will get into React Native with the next sync. Note that this code doesn’t live in react package so you’ll need to wait for react-native release including new code. I don’t know their schedule for sure so it’s a bit hard for me to say if the sync will make it into next release or not. Anyway, congrats on the contribution!

[gwmccull]

@gaearon thanks for all your feedback!