Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected action order with synchronous epics #21

Closed
GrzegorzKazana opened this issue Sep 4, 2020 · 2 comments · Fixed by #26
Closed

Unexpected action order with synchronous epics #21

GrzegorzKazana opened this issue Sep 4, 2020 · 2 comments · Fixed by #26

Comments

@GrzegorzKazana
Copy link
Contributor

GrzegorzKazana commented Sep 4, 2020

Do you want to request a feature or report a bug?
Bug (?).

What is the current behavior
If action A triggers is synchronously mapped to action B in epic, reducer receives action in order B A.

What is the expected behavior
Actions that are mapped in epics should preserve dispatch order.

Affected use-cases
Suppose dispatching FETCH_REQUEST action which will trigger a http request in epic. If creating the request fails (due to i.e. malformed url/headers), an error will be emitted synchronously. This in turn may trigger a FETCH_FAILED action. Reducer will receive these actions in order: FETCH_FAILED, FETCH_REQUEST. State will transition to the pending state, even the request has already errored out.

Minimal reproduction

it('should send actions back to the action$ stream respecting dispatch order', () => {
    const aAction = { type: 'A' };
    const bAction = { type: 'B' };
    const someEffect: Effect<Action, {}> = (action$) =>
        action$.pipe(ofType(aAction.type), mapTo(bAction));

    const reducerMock = jest.fn((state = initialState, _action) => state);

    const store = createStore(reducerMock, someEffect);

    store.state$.subscribe();
    store.action$.next(aAction);

    expect(reducerMock).toBeCalledTimes(4);
    expect(reducerMock).toHaveBeenNthCalledWith(1, undefined, initStateAction);
    expect(reducerMock).toHaveBeenNthCalledWith(2, initialState, initAction);
    expect(reducerMock).toHaveBeenNthCalledWith(3, initialState, aAction);
    expect(reducerMock).toHaveBeenNthCalledWith(4, initialState, bAction);
});
// shamelessly stole existing testcase to show the behavior
// test fails, swapping order of 2 last actions makes it pass

Additional context
For reference, redux-observable handles this case correctly, which leads me to believe this is not intended behavior:
https://codepen.io/GrzegorzKazana/pen/eYZeQVJ?editors=1010
(check the console output for action logs)

Potential cause
My diagnosis is as follows: (could be wrong tho 🤷 )
effect$ is the first subscriber of action$ (subscription made during initialization).
state$ is the second subscriber of action$ (subscription made when user subscribes state$).
Therefore, effect stream will always receive actions before reducer, and if the effect synchronously dispatches another action, it will be immediately nexted to the action$ (even before the input action will be handled in the scan operator).

Potential solution
Option 1. (simple):
Delay actions from epics using observeOn and asyncScheduler. This will guarantee that all action$ subscribers (including state$) will be flushed before resulting action from epic will be dispatched.

Option 2. (not-so-simple):
Redesign the creation of state$ from action$ and effect$. For now, no suggestions come to my mind. But I am fine with option 1.

P.S.
Pardon my mixed usage of effect and epic

@grzegorz-bielski
Copy link
Owner

Sorry, I missed that! 🙇 (too much spam from dependabot)

I am aware of this issue but I've always had been sweeping this under the rug... 😄
Switching the scheduler seems like a valid solution.

I was planning to do a bigger refactor after TS 4.1 lands since it brings recursive conditional types and template literal types which would drastically simplify the typing in the lib. I might then bring a fix for this as well.

If this is really important I'll try to resolve it a bit sooner.

@GrzegorzKazana
Copy link
Contributor Author

Thanks for feedback!

It is not that urgent, I actually stumbled across it by accident. But I think it might be one of those cases that backfire when we least expect, and cause the user to start doubting their comprehension of rxjs and coding in general (hope I am not the only one who has been there 😅 ).

From my perspective, I both do not need it asap, but also do not want to rush you with the aforementioned refactor. If you agree I could open a pr with the observeOn solution, and feel free to get rid of it if it will be no longer necessary after the refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants