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

feat(effects): resubscribe to effects on error #1881

Merged
merged 6 commits into from
May 23, 2019
Merged

feat(effects): resubscribe to effects on error #1881

merged 6 commits into from
May 23, 2019

Conversation

alex-okrushko
Copy link
Member

@alex-okrushko alex-okrushko commented May 22, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

When Errors happen in the main Actions stream that error is reported and the stream is closed

Closes # #1851

What is the new behavior?

For {dispatch: true}, which is the default Effect option, if the error occurs in the main actions stream, the effect will be resubscribed. This could result in unusual behavior (in cases where operators like startWith are used).

Resubscriptions can be turned off by setting {resubscribeOnError: false} in the effect metadata.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

BREAKING CHANGE:

Prior to introduction of automatic resubscriptions on errors, all effects had effectively {resubscribeOnError: false} behavior. For the rare cases when this is still wanted please add {resubscribeOnError: false} to the effect metadata.

BEFORE:

login$ = createEffect(() =>
    this.actions$.pipe(
      ofType(LoginPageActions.login),
      mapToAction(
        // Happy path callback
        action => this.authService.login(action.credentials).pipe(
            map(user => AuthApiActions.loginSuccess({ user }))),
        // error callback
        error => AuthApiActions.loginFailure({ error }),
      ) 
    ));

AFTER:

login$ = createEffect(() =>
    this.actions$.pipe(
      ofType(LoginPageActions.login),
      mapToAction(
        // Happy path callback
        action => this.authService.login(action.credentials).pipe(
            map(user => AuthApiActions.loginSuccess({ user }))),
        // error callback
        error => AuthApiActions.loginFailure({ error }),
      )
    // Errors are handled and it is safe to disable resubscription 
    ), {resubscribeOnError: false });

Other information

@alex-okrushko
Copy link
Member Author

Also refactored createEffect a bit - removed overrides and sprinkled it with a bit of TS ✨magic

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 22, 2019

Preview docs changes for 8096171 at https://previews.ngrx.io/pr1881-8096171/

Copy link
Member

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The git commit message needs to indicate the breaking change also

projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/migration/v8.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/migration/v8.md Outdated Show resolved Hide resolved
@brandonroberts brandonroberts added the Needs Cleanup Review changes needed label May 22, 2019
alex-okrushko and others added 2 commits May 22, 2019 11:29
@ngrxbot
Copy link
Collaborator

ngrxbot commented May 22, 2019

Preview docs changes for c6b9ded at https://previews.ngrx.io/pr1881-c6b9ded/

@alex-okrushko alex-okrushko removed the Needs Cleanup Review changes needed label May 22, 2019
@ngrxbot
Copy link
Collaborator

ngrxbot commented May 22, 2019

Preview docs changes for 1ac0665 at https://previews.ngrx.io/pr1881-1ac0665/

modules/effects/spec/effects_metadata.spec.ts Outdated Show resolved Hide resolved
modules/effects/spec/effects_metadata.spec.ts Show resolved Hide resolved
modules/effects/src/effect_decorator.ts Show resolved Hide resolved
projects/ngrx.io/content/guide/effects/lifecycle.md Outdated Show resolved Hide resolved
projects/ngrx.io/content/guide/migration/v8.md Outdated Show resolved Hide resolved
@brandonroberts brandonroberts added the Needs Cleanup Review changes needed label May 22, 2019
alex-okrushko and others added 2 commits May 22, 2019 18:05
Co-Authored-By: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-Authored-By: Brandon <robertsbt@gmail.com>
@ngrxbot
Copy link
Collaborator

ngrxbot commented May 22, 2019

Preview docs changes for 2041346 at https://previews.ngrx.io/pr1881-2041346/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 22, 2019

Preview docs changes for cf21508 at https://previews.ngrx.io/pr1881-cf21508/

@alex-okrushko alex-okrushko removed the Needs Cleanup Review changes needed label May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants