batchUpdates does not work as expected

[anacierdem]

This was a feature we have been looking forward to! (We currently use our own not-so-good solution to make some of our state updates atomic) Yet I suspect it is not working as we expected it to behave (maybe we are expecting too much but reading the inline docs and code this is how I think it should be). Consider this simple example;

// fetch is a generic action we use to make async network calls, it does a setState and updates a value on store
dispatch(fetch)
  // then the consumer derives a new value and stores it
  .then((data) => setState(deriveNewData(data)))

With this, we do two independent setStates one in the generic action and one inside .then and thus we will have two independent updates; one with fetch status (e.g isCompleted: true) and one with the derived data. We are currently deferring the first set state to the next event loop via setTimeout and this provided a better behaviour on average. (i.e we do not set isCompleted before data is ready anymore, but now data is updated earlier. This is just a temporary hack on our side. We were planning to implement a middleware next if this was not on the library itself.

I expected defaults.batchUpdates = true; to batch these two updates, but it is not the case. The schedule method never has a QUEUE larger than 1 and I suspect this is because of Promise.resolve().then not deferring the callback to the next event loop but instead is processed on the same phase, before the consumer's then is executed. See here, chrome should behave very similar to that;

generally, when the event loop enters a given phase, it will perform any operations specific to that phase, then execute callbacks in that phase's queue until the queue has been exhausted or the maximum number of callbacks has executed.

Which implies an immediately resolved Promise's success callback is added to the current phase when the outerHandler is first executed. Even if res is called before it, the engine will register the inner function before the outer handler as it has already started executing the outerHandlerand will not defer the inner promise to the next loop.

new Promise(function outerHandler(res) {
  res();
  Promise.resolve().then(() => console.log('inner resolved'));
}).then(() => console.log('outer resolved'))

[albertogasparin]

It's great to get your feedback on this new behaviours! Indeed the current batching method is very "dumb" and the gain is mostly around multiple actions triggered is sequence: it allows Sweet-state to batch them (something hard to do with a middleware).

The good news is that I think I've found a better way: by using React scheduler. If you try react-sweet-state@next, the batching in reality schedules the updates, and it works much better because it's handled by (and with) React. It means however that it is no longer "just batched": React will decide if it has time to render each individual state update or batch them together, and also how far in the future. This might result in different "results" based on how busy the thread is, but it should be always the "best" decision.

I'm still tweaking the priorities, as the current @next build will wait until 5s before triggering state updates, which is a bit too much and could screw up components in some cases. I'm testing using UserBlockingPriority that has a wait time of up to 250ms, which is a much better default and should handle your case quite well.

In case you are curious, this is the PR https://github.com/atlassian/react-sweet-state/pull/108 and you can install @next version and change the imported priority from scheduler to see what I mean.

[anacierdem]

Yes, it should work work for multiple state updates in a single go with the current version. I have an idea on how we may solve it in the middleware for our case at least (didn't test this one yet when I see the update on RSS 😄 );

// In generic action;
const fetch () => () => {
  ...
  setState(new TransactionStart());

  setState({
    isComplete: true
  })

  // Consumer then handlers will do more setStates before this is fired and they'll get batched.
  setTimeout(() => {
    setState(new TransactionEnd());
  }, 0)
}

And in the middleware we will intercept and stop update (don't call next) if it is a TransactionStart, collect and merge changes in between and then trigger the update on TransactionEnd. I'm not sure this will work with the existing update middleware though, need to try first.

ImmediatePriority may as well work for our case if it is always deferred to the next event loop instead of being put onto the same by React. I'll also look into that and try @next to let you know, great to hear this is something worked on!

[albertogasparin]

v2.5.1 has been published and batching is now more powerful.

[anacierdem]

It works much better now! I really wish they also have an "DeferredImmediatePriority". That would save us 250 more millis! :) Thank you for the update!

[anacierdem]

Hmm. That was a false positive. Somehow this is still not working as expected. I'm investigating further.

[anacierdem]

Hmm. That was a false positive. Somehow this is still not working as expected. I'm investigating further.

It is working totally fine, it was a configuration issue the second time 👍