Render re-entering after async function

[k1w1]

React Easy State version: 6.3.0
Platform: browser

There is another subtle problem after calling an async function. Array mutation causes immediate re-rendering for each fundamental operation (set, get, etc). This will cause the list to re-render while it is in an internally inconsistent state.

import React from 'react';
import { store, view } from '@risingstack/react-easy-state';

async function longFunction() {}

const counterStore = store({
  counters: []
});

counterStore.counters.push({value: 1});
counterStore.counters.push({value: 2});
counterStore.counters.push({value: 3});

export default view(() => {
  const updateList = async () => {
    await longFunction();
    
    const c = counterStore.counters;
    console.log("Before removing item")
    c.splice(0, 1);
    console.log("After removing item")
  }

  console.log("Current list:")
  counterStore.counters.forEach((counter, index) =>
    (console.log(`${index}: ${counter.value}`))
  );

  return (
    <>
      <button onClick={() => updateList()}>Update list</button>
      <ul>
        {counterStore.counters.map((counter, index) => (
          <li key={index}>{counter.value}</li>
        ))}
      </ul>
    </>
  );
});

Which generates the output:

Current list:
0: 1
1: 2
2: 3
Before removing item    <- Button is clicked
Current list:
0: 2                                  <- Note that value 2 is duplicated
1: 2
2: 3
Current list:
0: 2
1: 3                                    <- Now value 3 is duplicated
2: 3    
Current list:
0: 2                                    <- length is updated so list is correct again
1: 3
Current list:
0: 2
1: 3
After removing item

In this output we are seeing the internal implementation of splice as it copies down the array elements, and then finally updates the length. This is a problem for React rendering because if the value is being used as a key value then the React display will become corrupted because of the duplicated keys. There will be a warning, but then you will also see duplicated items even after all of the renders have finished.

It seems that some kind of batching is required. There is batching around the event callback, but the async function closes the batch and subsequent changes are not batched.

[solkimicreb]

This should work 🤔 See the exact code copied into codesandbox. Event handlers (even async ones) have automatic batching applied to them (by Easy State, not the default React one). Am I missing something? Which browser are you using? Are you using create-react-app? Do you transpile async functions or do you leave them intact?

Some context about the current state of batching and future plans:

At the moment React setState is always causing a synchronous rerender. In case of React event handlers, the render is executed only after the whole event handler finished execution. Otherwise, it is executed right after the setState call.

Easy State is triggering rerenders by calling dummy setStates so it also works synchronously. It applies synchronous batching for most use cases though (just like React event handlers and setState):

• When stores are mutated from store methods.
• When stores are mutated from event handlers.
• When stores are mutated from pretty much any task source.
• When stores are mutated inside a batch wrapper.

This means that the only usual exception is when stores are mutated from the global scope. (I don't understand why your use case is not batched).

Not having batching is especially ugly when Proxies are involved, as you already found out. It may expose some internal states when a seemingly atomic operation (array splice) is implemented in JavaScript by the platform.

I tried a few different iterations until I arrived at the current batching solution. For example, async batching added a new task with setTimeout/Promise/etc after the current sync piece of code finished executing and flushed all reactions after. This was nice but changed the sync nature of React and caused some strange side effects.

The current status of batching is this:

• React is still promising to switch to a fully async batching mode with the Async React release. This is what other popular frameworks - like Vue and Angular - do btw. After Async React is finally released this will become obsolete. That might take a while though.

• I don't plan to add any more auto batching rules, I am in fact trying to get rid of them. You can use the batch wrapper in cases where auto batching is not applied.

• I am trying to make both auto-batching and batch obsolete by the introduction of strict mode in the next release. Strict mode will force the user to place all store mutations in store methods and store methods are automatically batched. I am a bit worried about this being a bit too restrictive so I am testing different scenarios. What is your opinion?

[k1w1]

I am trying to find standard patterns for our team to write React code. I am frustrated by the limitations in most React code with what actions can be performed in which contexts. That is the attraction of react-easy-state - better than any other solution I have found - it frees the developer from the implementation level details of hooks, state stores and immutability.

Having some limitations does seem reasonable, so for example, limiting store mutations to be within special store methods would be OK. However, not being able to use await within those methods would be a severe limitation. Being able to write linear, imperative, code is the key simplifying principle I am looking for.

I want to be able to write code like this, and have it just work:

function actionLoadData() {
   state.loading = true;
   state.records = await FetchRecordsFromServer();
   state.recordTree = ArrangeIntoTree(state.records);
   state.loading = false;
}

Currently this does appear to work. Perfectly. React re-renders in the loading state while waiting for the data from the server, and then renders again after the data is returned. The problem is that after the await react is actually re-rendering many times, which is fine in many cases, and is not really visible to the end-user, but can have subtle problems like the splice one above.

In the ideal world there would be batching between each await. So that all changes which are made together are more efficient, and more importantly are atomic - which is what the naive developer is expecting.

I am not sure how to inject a batch wrapper between the awaits. I could imagine it being possible with babel. An alternative would be to have the default batching such that it collects changes until the Javascript main-loop starts again (i.e. using setTimeout(... ,0))

BTW, adding a batch call around the splice call does fix the problem. But IMHO requires too much careful thought on the part of the developer to get right consistently.

[solkimicreb]

I updated my comment a few times after posting it, sorry about that ): I did not read your original issue carefully enough, so I am posting the (recently added) codesandbox repro here again. Your use case should work, works in the above sandbox, and I have no idea why it does not work for you.

Some questions about it:

• Which browser are you using?
• Are you using create-react-app?
• Do you transpile async functions or do you leave them intact?

---

However, not being able to use await within those methods would be a severe limitation.

Yep, I agree.

In the ideal world there would be batching between each await. So that all changes which are made together are more efficient, and more importantly are atomic - which is what the naive developer is expecting.

This should happen at the moment. Promises are monkey patched to make this work (it's ugly but it works until Async React arrives).

An alternative would be to have the default batching such that it collects changes until the Javascript main-loop starts again (i.e. using setTimeout(... ,0))

There was a version with async batching like this. It was technologically much simpler but it caused a whole range of issues. Mixing sync React with an async scheduler was not a good idea and I switched back to sync batching. We will have to wait until Async React arrives with this, I am afraid.

Adding a batch call around the splice call does fix the problem. But IMHO requires too much careful thought on the part of the developer to get right consistently.

Agreed again. I am trying to gradually get rid of that ugly function and I try to not promote it too much in the meantime.

[k1w1]

I reproduced in repl.it here https://repl.it/@k1w1/WeepyBluevioletUsername using Chrome 81 on OSX.

I observed the same problem in my application code which is babel transpiled.

I see that the codesanbox.io example you linked works. I am having trouble getting codesandbox.io to work properly in order to understand why it works and what is different.

[k1w1]

It looks like the difference is whether an async polyfill is being used. codesandbox.io is using a polyfill:

repl.it is not using a polyfill:

Where the polyfill is not being used then the react-easy-state hook is not working.

[solkimicreb]

Thanks, that was pretty helpful!

This is considered a bug indeed and I will look into it to see if I can deploy a quick hotfix before the next stable release (which will still take some time).

Some info about my related plans:

• Plan A: check if I can hook into / monkey patch something other than Promise to make this work.
• Plan B: readd the async batching you also mentioned (with a timeout) as a second fallback iteration. It will run after the current sync batching and apply an extra batching round to anything that was missed by the sync round. This way it won't break any current behavior but it will fix anything that the current sync batching can not batch. This won't have the previously mentioned sync/async issue since the related code is already async and 'a bit more async-ness won't hurt'.
• Plan C (longer term): try switching to fully async batching again with all the knowledge I gained from the last iterations.

EDIT: This will probably not affect you in practice if you are using create-react-app in the meantime. I just checked and it transpiles async/await even if browserlist is set to "latest 2 Chrome"

[solkimicreb]

It's time to revisit batching again anyways. My opinion was to sit an wait for Async React so far but according to some recent tweets that will be a pretty long wait ):

[k1w1]

Plan B makes the most sense to me: If a reaction is triggered outside of a batch, then create a new batch that is scheduled to run the next time the main loop runs (with setTimeout). While an async batch is open add any other reactions to that batch too.

This might change the behavior that someone is observing today (e.g. a loop triggering renders that happens after an async call), but I think that in general it would give the programmer the behavior they expect from Javascript: a sequence of pure data manipulation can never be interrupted by another thread of execution. (The other thread in this case being the react rendering).

BTW, plan A is not enough. It will fix this async case, but I have another example of exactly the same problem that happens because of events triggered from react-dnd which are not wrapped in a batch either.

[luisherranz]

@solkimicreb what is your plan to add batching between awaits? Use the Set handler to start the batching? If it's not, may I know why did you ditch that idea in the first place?

[solkimicreb]

what is your plan to add batching between awaits? Use the Set handler to start the batching? If it's not, may I know why did you ditch that idea in the first place?

This is an open discussion for now, I am playing with different options.

The issue is not really about starting the batch but ending and flushing it. Using the set handler and the scheduler to start the batch are pretty much equivalent (and the lib is doing the latter right now).

Right now the lib patches a lot of task sources and flushes the batches at the end of them, which is a form of synchronous batching. The issue with this is that new unpatched task sources keep coming up - like the untranspiled async/await case.

I am planning to switch back to async batching (again). Which starts a new batch on the first set operation and flushes it in the very next microtask (Promise.resolve().then(flush)). This behaves differently from the sync behavior of React though, which will be very apparent during testing. See the below pseudo-code:

<div>{store.name}</div>

store.name = 'Ann'
// this will fail because the div is not re-rendered synchronously but only in the next microtask
expect(div.textContent).toBe('Ann')

Either of the below options will work:

import { flush } from '@risingstack/react-easy-state'
store.name = 'Ann'
// this is the recommended way, it flushes all reactions synchronously
flush()
expect(div.textContent).toBe('Ann')

or

import { flush } from '@risingstack/react-easy-state'
store.name = 'Ann'
// this is a hacky way, we wait for async batching to be flushed
await Promise.resolve()
expect(div.textContent).toBe('Ann')

Apart from testing, it should not have any apparent effect in React apps. I am still testing this and hunting for edge-cases.

---

This will also likely happen to React when Async React arrives (they already have the act testing helper) and I originally wanted to wait for that instead with this switch. It seems to take a long time though.

I am also playing with re-introducing some sync batching after the async switch but it seems to be a bit chaotic. The main idea is to keep all the current sync patched batching and if something is not patched by sync batching fall back to the async one.

The issue with this one is that it is not well defined. The currently buggy/unpatched codes (like mutating stores from the global scope) will be async and everything else will be sync, which is a bit random.

Nothing is set in stone yet. What is your opinion about this?

[k1w1]

Your description above makes sense to me. From the debugging I have been doing in my real-world app I think it would solve the edge cases in a way that is expected.

I think that needing to call flush() in tests is reasonable, and would be expected in a React app where we wouldn't normally expect any state change to be immediately reflected in the DOM.

Having a generic approach to batching that doesn't rely on patching specific functions also seems more robust from a long term perspective. The only situation where it would fail is where the developer does not let execution return to the event loop - but that would be a problem in any case, completely independent of react-easy-state.

[luisherranz]

Gotcha. Thanks for the explanation @solkimicreb.

I think doing async flushing on React is fine, and as you say, it's going to be the default once they release the Concurrent mode anyway...

Also, using flush in tests don't seem so terrible. People is already used to act as well.

[solkimicreb]

Thanks for your opinions. Next alpha (or experimental) with the async batching is coming soon, so you can try it. I will let you know here.

[k1w1]

I am coming back to this issue. We are using react-easy-state quite extensively now. It is fantastic, but this batching problem still bites us. When it does happen it is extremely difficult to diagnose and then work out where to place the batch call.

I think that plan B you proposed above is still the right approach.

It is worth noting that I don't think plan A (or any monkey patching scheme) will work long term. As we support only newer browsers we are removing polyfills for things like Promises. The native Promise can't be monkey patched. We had some other code that depended on patching promises that no longer works with native promises.

Anyway, I would love to see this batching code come to fruition. React-easy-state is so close to perfect.

[luisherranz]

I've been thinking a bit more about this in the last weeks and I am starting to think that moving to async batching may not be the best idea after all.

The first reason is that the new Concurrent mode will probably introduce one (or more) APIs to handle different types of rendering. If those APIs finally follow a similar pattern to useTransition, then the internal setState() calls that happen inside the startTransition() callback will need to be synchronous.

import { store, view } from "@risingstack/react-easy-state";

const router = store({
  page: 1
  get url() {
    return `/page/${router.page}`;
  },
  nextPage: () => {
    router.page += 1;
  },
});

const App = view(() => {
  const [startTransition, isPending] = useTransition();

  return (
    <>
      <button
        disabled={isPending}
        onClick={() => {
          startTransition(() => {
            // This change needs to trigger a sync setState() or
            // React won't link the rerender to the transition,
            // and things like `isPending` won't work.
            router.nextPage();
          });
        }}
      >
        {!isPending ? "Next Page" : "Loading..."}
      </button>

      <Suspense fallback={<Spinner />}>
        <Page url={router.url} />
      </Suspense>
    </>
  );
});

I am not 100% sure about this, but I think that's the way useTransition works.

The second reason is that the new Concurrent mode will do async batching by itself, so doing async batching again in react-easy-state will be unnecessary. These is from the current Concurrent docs:

Legacy mode has automatic batching in React-managed events but it’s limited to one browser task. Non-React events must opt-in using unstable_batchedUpdates. In Blocking Mode and Concurrent Mode, all setStates are batched by default.

So not only the async batching will be unnecessary, but also the scheduler, because React will take care of all the batching.

But those changes don't apply until React Concurrent is finally released and until that moment, there is no way to batch native async/await functions until without react-easy-state async batching.

Async/await functions don't use to be a problem, but due the introduction of the <script type="module"> support of modern browsers, the module bundles don't transpile async/await functions anymore.

This problem, however, can be solved by the user with batch:

import { store, batch } from "@risingstack/react-easy-state";

const user = store({
  fetching: false,
  status: "idle",
  userId: null,

  getUser: async () => {
    // There is no need to batch the mutations that happen before
    // the first await.
    user.fetching = true;
    user.status = "getting user id";

    const { id } = await fetch(USER_API).then(r => r.json());

    // But we need to batch the mutations that happen after awaits to
    // avoid multiple rerenders and inconsistent UI state.
    batch(() => {
      user.fetching = false;
      user.status = "finished";
      user.userId = id;
    })
  },
});

So I guess the question is:

• Should we avoid this structural change and assume that react-easy-state is not going to batch native async/await functions, and promote the use of batch() in those functions?
• Or should we change to async batching and remove it once React Concurrent is released?

I'd love to hear your opinions @k1w1 @solkimicreb 🙂

[k1w1]

My philosophy is that we cannot be paralyzed while waiting for Facebook to complete concurrent mode; we need to keep moving forward.

My PR handles batching automatically, and completely without the developer knowing it is happening, which seems to me to be the whole point of react-easy-state. We have been using the code in this PR for a while now and it works very well. We have not discovered any caveats or performance impacts.

So for our project we are already following your second option:

Or should we change to async batching and remove it once React Concurrent is released?

If and when React Concurrent solves this problem we can use that, and if it doesn't we can keep using the microtask based batching. For our developers using explicit batch() just didn't work. It was too hard to know that the context of the current code required batching. In real-world applications where there are deeply nested functions, and a lot of code-reuse it is almost impossible to keep track of whether the current execution is in an event callback, or setTimeout or promise, etc.

[bminer]

I'd like to resurrect this issue yet again, years later. Batching is biting me, as well. Now with React 18 released, what do you guys think is the best course of action moving forward?

[k1w1]

We are still using the code in the PR I linked above with great success. We use react-easy-state extensively and our team loves how quickly it allows us to write very complex, stateful, code. The transparent batching in the PR completely solves the issue.

We are still using React 16 and have not evaluated how React 18 might make it better.