rt2zz / redux-action-buffer

Buffer Redux Actions
MIT License
79 stars 6 forks source link

Order of actions dispatched within promises in v1.1.0 #12

Open benjaminmock opened 7 years ago

benjaminmock commented 7 years ago

I discovered some strange behaviour with the 1.1.0 release. When dispatching actions within promises the resulting order is no longer correct (or actions are missing). The problem seems to be caused by the use of setImmediate/setTimeout.

I tried to replicate this behaviour in the following test (works with 1.0.1 fails for 1.1.0):

import test from 'ava'
import { createStore, applyMiddleware } from 'redux'
import actionBuffer from '../index'

test('buffers async actions in correct order', t => {
    const actionHistory = []

    const store = createStore(
        (state, action) => {
            if (action.type.indexOf('@@') !== 0) actionHistory.push(action)
            return {}
        },
        null,
        applyMiddleware(actionBuffer('BREAKER'))
    )

    const action1 = {type: 'ACTION1'}
    const action2 = {type: 'ACTION2'}
    const breaker = {type: 'BREAKER'}

    store.dispatch(action1)

    return new Promise(resolve => {
        store.dispatch(breaker)
        store.dispatch(action2)
        resolve()
    }).then(() => {
        t.is(actionHistory[0], breaker)
        t.is(actionHistory[1], action1)
        t.is(actionHistory[2], action2)
    })
})
rt2zz commented 7 years ago

I think this commit broke it: https://github.com/rt2zz/redux-action-buffer/commit/d5e7222c64a00ba177bdaf8ac1dc179244d366e8

likely due to the setImmediate usage. I will revert.

rt2zz commented 7 years ago

actually @benjaminmock I am not sure what the best resolution here is. That PR solved an issue with redux-persist. Is it incorrect if the queue drains on setImmediate? I am uncertain but open to input. Your test should be fixed with

    return new Promise(resolve => {
        store.dispatch(breaker)
        store.dispatch(action2)
        setTimeout(resolve, 1)
    }).then(() => {
        t.is(actionHistory[0], breaker)
        t.is(actionHistory[1], action1)
        t.is(actionHistory[2], action2)
    })
rt2zz commented 7 years ago

note: I agree this is messy, just not sure what best resolution is. We can probably fix the original issue in redux-persist but that will take more time and care.

benjaminmock commented 7 years ago

@rt2zz Thank you for your input. So far I could not come up with a clean solution either.

The test covers a perfectly valid use case. action1 triggers a loading indicator, action2 hides it in a promise, that is returned by the http client. With the current release the loading indicator is shown after the request returned successfully. Fixing this with another setTimeout feels quite wrong (and may even be not possible in every case).

For now my fix is to just use your 1.0.1 release. But I'll have a look at the code again.

rafatwork commented 6 years ago

Any news on this?

Also experiencing this issue, 1.0.1 does seem to fix it for me as well. Unfortunately, I do use redux-persist, so 1.1.0 would be a betterf fit in my situation.