`<Async.Fulfilled>`'s function gets called with a mix of old&new data on subsequent updates

[slawomir-brzezinski-at-clarksons]

Hi. This seems like a new case for #8 , except that one was closed based on side effects in render. Here we have a valid side-effect in componentDidUpdate that gets triggered unexpectedly.

Essentially, we have a

loadRootEntityAsync({ rootEntityId }) {...}

render:
              <Async
                promiseFn={this.loadRootEntityAsync}
                rootEntityId={rootEntityId}
              >
                <Async.Fulfilled>
                  {rootEntity => (
                    <RootEntity
                      rootEntity={rootEntity}
                      selectedChildEntityId={selectedChildEntityId}
                    />
                  )}
                </Async.Fulfilled>
              </Async>

Note that selectedChildEntityId can be null, when the user doesn't navigate using a precise URL, but then there's some logic in <RootEntity> that will select the appropriate child entity - this logic happens in <RootEntity>'s componentDidUpdate.

So, it works perfect on the show of the first root entity, but when user navigates to another root entity (by changing rootEntityId and setting selectedChildEntityId to null), the old <Async.Fulfilled>, just before the <Async.Pending> for the new fetch, gets another invocation with the old rootEntity, and empty selectedChildEntityId, which triggers a new finding of selectedChildEntityId, resetting the user navigation. This is the usual shape of the problem, but if the user navigates straight to a specific selectedChildEntityId (e.g. using browser back button or using a direct link) it will result in creating a component tree where the new child entity is in the context of an old root entity. This, will throw an error in the best case, but might as well cause corruption, depending on what the components do in their lifecycle methods.

For now, the workaround is to use <Async key={rootEntityId}>, a common technique for preventing components reuse, which prevents that last unnecessary firing of the <Async.Fulfilled>'s function.

It seems though this could be prevented elegantly by preventing the render of the <Async.Fulfilled>'s function the moment the arguments to promiseFn change. Instead of that one, <Async.Pending> could be used in this render, which, semantically, seems to be even more correct - another execution of the async action is imminent.

[slawomir-brzezinski-at-clarksons]

Another idea to fix this, perhaps a simpler one, is to use the same approach as <Async key={rootEntityId}> but do it inside the <Async> itself - extract some kind of private <AsyncCallInstance key={...}>, that you will only use in the class behind the <Async> component.

Regarding what to put in the key, given the difficultly from the fact that <Async> can have multiple arguments (so not sure if you can pass props to the key, as it's perhaps a new object each time), my idea would be that maybe you could just have a nextKey member variable in <Async> that increments on every componentDidUpdate, and use it for <AsyncCallInstance key={this.nextKey}> (thanks to being a member variable, not a state, the componentDidUpdate itself would not cause the render to fire again). componentDidUpdate already fires only when the properties change their values, so you should naturally get an unmount of the old <AsyncCallInstance> every time any property to <Async> changed.

[ghengeveld]

Hey Slawomir, I'd love to slightly optimize React Async's behavior to account for this scenario. However I don't have the time to really dive in. Could you at least provide a minimal reproduction of the issue?

I also think you might be able to avoid the issue altogether with useAsync, where you have full control over component rerendering. Have you tried that?

[slawomir-brzezinski-at-clarksons]

I'm sorry, but also a bit busy. Will strive to attend to this this month :)

Re useAsync, it has not been a preference so as to represent UI structure with markup as much as we can, so the key= workaround seems a more lighweight one.

[slawomir-brzezinski-at-clarksons]

Hi @ghengeveld . Re:

Could you at least provide a minimal reproduction of the issue?

I have now created a branch in my fork (diff view) that adds a failing test.

[gamliela]

I have the same problem, but with the useAsync hook. After changing the promise in the hook, I'm getting back the old data (of the previous promise).
Any plans to fix this? Seems pretty critical IMO!
I'm not sure about the status of this project, but if still maintained I can try and contribute a PR to fix it.