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

Silent unsubscription of all Effects if one completes. #232

Closed
jinder opened this issue Aug 3, 2017 · 4 comments
Closed

Silent unsubscription of all Effects if one completes. #232

jinder opened this issue Aug 3, 2017 · 4 comments

Comments

@jinder
Copy link
Contributor

jinder commented Aug 3, 2017

I'm submitting a...


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

What is the current behavior?

When there is an @Effect observable that completes (for example, has a take(1)), NGRX will silently unsubscribe all @Effect observables in that class.

Expected behavior:

Shouldn't unsubscribe any unaffected observables at all.

Minimal reproduction of the problem with instructions:

Create an Effect class with two annotated effects, and ensure one completes after the first action by adding take(1) to it. The other observable (which has not completed) will no longer receive any items.

Plunkr: http://plnkr.co/edit/BWXu3gm4ULC2W1TQ8UH7

Version of affected browser(s),operating system(s), npm, node and ngrx:

@ngrx 4.0.2

Other information:

@jinder
Copy link
Contributor Author

jinder commented Aug 4, 2017

Have provided a plunkr here that demonstrates the problem (see app.ts): http://plnkr.co/edit/BWXu3gm4ULC2W1TQ8UH7

Open your debug tools to view console output. Two intervals have been set up to dispatch actions.

Once the DECREMENT action is dispatched, the decrement$ effect takes one item, and then maps it to a new action. At that point, all effects are unsubscribed. This only occurs if there's a map (if you set dispatch: false, it won't happen).

@phillipzada
Copy link
Contributor

Hey @jinder

With effects they are pretty much a "shared" subscription of the injected action. If you want to use take within an effect your best best then will be to have multiple effects one effected by the take and the other not.


@Injectable()
export class TestEffects {
  
  skipped = false;
  
  constructor(private _actions$: Actions) {}
  
  @Effect({ dispatch: false })
  increment$ = this._actions$
    .ofType(fromCounter.INCREMENT)
    .do(x => console.log(x));
    
  @Effect({ dispatch: false })
  reset$ = this._actions$
    .ofType(fromCounter.RESET)
    .do(x => console.log(x));
    
}


@Injectable()
export class TestEffects2 {
  
  constructor(private _actions$: Actions) {}
  
  @Effect()
  decrement$ = this._actions$
    .ofType(fromCounter.DECREMENT)
    .take(1)
    .do(x => console.log(x))
    .map(() => ({ type: fromCounter.RESET }));
}

@jinder
Copy link
Contributor Author

jinder commented Aug 8, 2017

@phillipzada Thanks for the workaround.

I think the current behaviour is quite wrong. It seems counter-intuitive that two unrelated Observables can impact one another in this way.

@brandonroberts @MikeRyanDev do you consider this to be a bug, or intended behaviour? I can work on a PR if you think it's a bug.

@MikeRyanDev
Copy link
Member

Sounds like a bug to me

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

3 participants