redux-async-connect icon indicating copy to clipboard operation
redux-async-connect copied to clipboard

loadState overwritten for each request

Open kjanoudi opened this issue 9 years ago • 4 comments
trafficstars

If i use reduxAsyncConnect in multiple components that are being rendered, loadState is overwritten in some situations (race condition). Rather than replace loadState as is being done now:

case LOAD:
      return {
        ...state,
        loadState: {
          [action.key]: {
            loading: true,
            loaded: false
          }
        }
      };

i suggest merging loadStates and only replacing the keys that match.

kjanoudi avatar Feb 26 '16 07:02 kjanoudi

Js is one threaded, therefore loop is blocked and 2 "async" actions won't ever call reducers with the same state twice

On 26 Feb 2016, at 10:17, kjanoudi [email protected] wrote:

If i use reduxAsyncConnect in multiple components that are being rendered, loadState is overwritten in some situations (race condition). Rather than replace loadState as is being done now:

case LOAD: return { ...state, loadState: { [action.key]: { loading: true, loaded: false } } }; i suggest merging loadStates and only replacing the keys that match.

— Reply to this email directly or view it on GitHub.

AVVS avatar Feb 26 '16 08:02 AVVS

@AVVS while that's true, it seems that by not using the Object.assign method of setting the new state, the loadState key is replaced fully. So you will never be able to have:

state: {
  loadState: {
     config: { loading: true, loaded: false},
     users: {loaded: true, loading: false}
  }
}

instead only able to have either loadstate.config or loadstate.users but never both

rather than returning this:

return {
        ...state,
        loadState: {
          [action.key]: {
            loading: true,
            loaded: false
          }
        }
      };

i think this would solve it:

return _.merge({}, state, {
            loadState: {
               [action.key]: {
                   loading: true,
                  loaded: false
               }
            }
        })

kjanoudi avatar Feb 26 '16 08:02 kjanoudi

Thats true. But its not a race condition, just lack of deep merge. Should do spread assign to this property as well and that would solve it

On 26 Feb 2016, at 11:48, kjanoudi [email protected] wrote:

@AVVS while that's true, it seems that by not using the Object.assign method of setting the new state, the loadState key is replaced fully. So you will never be able to have:

state: { loadState: { config: { loading: true, loaded: false}, users: {loaded: true, loading: false} } } — Reply to this email directly or view it on GitHub.

AVVS avatar Feb 26 '16 09:02 AVVS

@kjanoudi , yes, looks like a bug. Any PR or help appreciated. Or i'll fix it by myself and a little bit later

sars avatar Feb 29 '16 13:02 sars