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

Actions processing order inside effects after upgrading to version 4.x #309

Closed
maglianim opened this issue Aug 22, 2017 · 13 comments
Closed

Comments

@maglianim
Copy link

maglianim commented Aug 22, 2017


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[x] Support request

Hello, After updating the module @ngrx from version 2 to 4 the actions inside effects of my application are processed in a different order and I believe this is due to the changes made to close the issue #43 (reported as a bug). consider this code:


    @Effect()
    rootEffect$: Observable = this.actions$
        .ofType(system.ActionTypes.ROOT)
        .map((action: RootAction) => action.payload)
        .switchMap(data => {
            return Observable.concat(
                of(new ActionA()),
                of(new ActionB()),
                of(new ActionC())
            );
        });

    @Effect()
    childEffects$: Observable = this.actions$
        .ofType(system.ActionTypes.ACTION_A)
        .switchMap(data => {
            return Observable.concat(
                of(new ActionA1()),
                of(new ActionA2()),
                of(new ActionA3())
            );
        });

Before the upgrade i got the sequence ACTION_A, ACTION_A1, ACTION_A2, ACTION_A3, ACTION_B, ACTION_C (children before root) as a consequence of processing action RootAction

After the upgrade the resulting order is: ACTION_A, ACTION_B, ACTION_C, ACTION_A1, ACTION_A2, ACTION_A3 (root before children). Is this behavior the correct one?
As the first sequence is the one that works in my application, do I have to remove action nesting inside effect in order to restore it?

Thank you.

@mmaask
Copy link

mmaask commented Aug 22, 2017

Having similar issue. Not sure if intended or not, but it breaks the current behavior I have. Any clarifications would be appreciated.

@jinder
Copy link
Contributor

jinder commented Aug 23, 2017

If the order matters, you should probably dispatch actions after a previous dependent action has completed. Therefore Action A should have enough information to dispatch the next action when it is handled.

@maglianim
Copy link
Author

maglianim commented Aug 25, 2017

Hello,

There are some cases where an action couldn't know what to dispatch after it's handled. Let's suppose it's performing some general task, such as reset the application layout to an initial state. there could be different situations needing the layout to be reset before executing some other tasks. For example:

@Effect()
resetUiState$: Observable<Action> = this.actions$
    .ofType(system.ActionTypes.RESET_UI_STATE)
    .switchMap(data => {
        return Observable.concat(
            of(new ResetFoo()),
            of(new ResetBar())
        );
    });

@Effect()
performTask1$: Observable<Action> = this.actions$
    .ofType(system.ActionTypes.PERFORM_TASK_1)
    .switchMap(data => {
        return Observable.concat(
            of(new resetUiState()),
            of(new loadDataTask1()),
            of(new doOtherThingsTask1())
        );
    });
    
@Effect()
performTask2$: Observable<Action> = this.actions$
    .ofType(system.ActionTypes.PERFORM_TASK_2)
    .switchMap(data => {
        return Observable.concat(
            of(new resetUiState()),
            of(new loadDataTask2()),
            of(new doSomeOtherThingsTask2())
        );
    });

So far the only solution i found to ensure the correct execution order is to get rid of "RESET_UI_STATE" and call its children explicitly when handling the actions "PERFORM_TASK_1" e "PERFORM_TASK_2". It works but it is a violation of DRY principle; Furthermore, if in a future version i need to reset a new component i'll have to modify the code in every method that need the ui to be reset. Any suggestion?
Thank you

@jinder
Copy link
Contributor

jinder commented Aug 31, 2017

@maglianim You could have a variant ResetUiState action that has an option that indicates what subsequent action to dispatch. It could be as crude as having a property that contains the next action type and payload which is dispatched by your effect.

The fact that your data task requires an initial UI reset suggests a problem with your overall architecture - although difficult to say for sure with your example.

@maglianim
Copy link
Author

maglianim commented Sep 1, 2017

@jinder My specific problem was that I have a collapsable panel where i load a different components according with user's gestures. Before opening the selected component, an action was dispatched to reset the previous panel state (closing previous opened components and collapsing the panel). I use the same reset action when the user wants to explicitly choose to close the detail panel. Everything worked until the new version of ngrx where the panel was collapsed after openening the desired component because now nested actions are processed before root actions, as documented in issue #43. I solved the problem changing the behavior of the reset action in a way that works regardless the processing order.

@brandonroberts
Copy link
Member

@maglianim closing as non-actionable.

@xaralis
Copy link

xaralis commented Dec 6, 2017

@brandonroberts
This is really strange. When you have scenario like this:

@Effect()
oauthLoginAsSomeoneElse$: Observable<Action> = this.actions$
    .ofType<AuthActions.OAuthLoginAsSomeoneElse>(AuthActions.OAUTH_LOGIN_AS_SOMEONE_ELSE)
    .pipe(
        concatMap(action => [
            // First logout
            new AuthActions.OAuthLogout(),
            // Then login again
            new AuthActions.OAuthLogin({redir: action.payload.redir})
        ])
    );

@Effect({dispatch: false})
oauthLogin$: Observable<Action> = this.actions$
    .ofType<AuthActions.OAuthLogin>(AuthActions.OAUTH_LOGIN)
    .pipe(
        map(action => oauthRedir.start(
            this.authEndpoint + '/oauth/authorize',
            this.authClientId,
            this.authRedirectUri,
            this.appBaseHref,
            action.payload.redir,
        ))
    );

@Effect()
oauthLogout$: Observable<Action> = this.actions$
    .ofType<AuthActions.OAuthLogout>(AuthActions.OAUTH_LOGOUT)
    .pipe(
        switchMap(action =>
            this.http.post(`${this.authEndpoint}/logout`, null, {withCredentials: true}).pipe(
                map(responseData => new AuthActions.SessionTerminate()),
                catchError((err: HttpErrorResponse) => {
                    return of(new RouterActions.Go({path: ['/']}));
                }),
            )
        )
    );

You would expect, that when dispatching OAuthLoginAsSomeoneElse everyting related to OAuthLogout action will be finished (including some async stuff in actions it's @effect() emits) before emitting OAuthLogin again. I mean, first process first root action and it's children, then process next emitted root action. It's very inconvenient to supply chain of actions to perform next as parameter, as you would need to pass it down to the children too, like to SessionTerminate in case it's async (making the code real mess). This just doesn't scale right.

How am I supposted to handle such situations without tons of boilerplate code?

I might be missing something - this is all within same module. Maybe it's not related to #37 or #43 in fact.

@jinder
Copy link
Contributor

jinder commented Dec 6, 2017

@xaralis Why would you expect that? Your logout performs an async POST operation, which won't return immediately. Any other actions will get processed in the meantime.

@xaralis
Copy link

xaralis commented Dec 6, 2017

@jinder It feels natural. I know it's doing some async stuff. Do you have any proposal on how to handle such situations? Except for the mentioned workaround of passing next action to the OAuthLogout. This really soon becomes quite tedious when it triggers some other chain of actions.

@jinder
Copy link
Contributor

jinder commented Dec 6, 2017

@xaralis It wouldn't feel natural to me. The whole benefit of observables/promises/async is that you can do stuff while waiting for IO completion.

I've found that if you often need lots of follow up actions that depend on order, your architecture is likely to be wrong. In your particular instance, why does your OAuthLoginAsSomeoneElse action not handle the logging out/logging in altogether? Your actual code for logging in/logging out shouldn't really be part of your effect - you should stick it in a service, and use your effect to link actions to your service.

@xaralis
Copy link

xaralis commented Dec 6, 2017

@jinder OK, I see. Well, if this was possible, it would definitely feel way more DRY. This way, I will have to copy&paste some parts of code just because I cannot define the sequence in which actions are dispatched.

I totally get paralelism of actions, I just would like to be able to control what's happening and allow some blocking in cases when it's desired (like here).

This example is of course simplified just to be transparent of what it does. Naturally, in real world scenario, you would use a service to do the heavy lifting.

OK, we can probably close with that, it's shame something like that is not possible.

@jinder
Copy link
Contributor

jinder commented Dec 6, 2017

@xaralis this wouldn't violate DRY (pseudocode):

@Effect()
oauthLoginAsSomeoneElse$: Observable<Action> = this.actions$
    .ofType<AuthActions.OAuthLoginAsSomeoneElse>(AuthActions.OAUTH_LOGIN_AS_SOMEONE_ELSE)
    .switchMap => service.logout
    .switchMap => service.login

@Effect({dispatch: false})
oauthLogin$: Observable<Action> = this.actions$
    .ofType<AuthActions.OAuthLogin>(AuthActions.OAUTH_LOGIN)
    .switchMap => service.login

@Effect()
oauthLogout$: Observable<Action> = this.actions$
    .ofType<AuthActions.OAuthLogout>(AuthActions.OAUTH_LOGOUT)
   .swithcMap => service.logout

@xaralis
Copy link

xaralis commented Dec 6, 2017

@jinder Thanks, I'll stick to your suggested approach.

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

No branches or pull requests

5 participants