CODEWITHSUNDEEP
ngrx-effects
milesmeow
milesmeow

Reputation: 3767

NGRX effects - is it bad practice to dispatch actions for effects that have {dispatch: false}?

I have an effect that may or may not dispatch an action:

handleMainAccount$ = createEffect(() => {
    return this._actions$.pipe(
      ofType(setUserDetails),
      tap( (details) => {
          if (details?.mainAccount) {
            const fakeData = { foo: 'bar'};
            this._accountHttpSvc.updateAccount(fakeData).pipe(
              take(1), // so we don't have to unsub
              map((res) => {
                this._userStore.dispatch(setMainAccount({fakeData}));
              }),
            ).subscribe();
          }

          // else 
          // we are not dispatching any action

      })
    );
  } , { dispatch: false } ); // setting as false, because we may or may not dispatch an action

I have dispatch as false, because some times this effect may not dispatch an action. Is this bad practice? What would be the negative impact if any?

Upvotes: 3

Views: 3794

Answers (1)

Michele Stieven
Michele Stieven

Reputation: 562

Imho it's a bad practice. I divide effects in 2 categories:

  1. Effects which emit actions
  2. Effects which don't

Yours is kinda "both". It becomes more difficult to test and manage if requirements should change.

Also you're subscribing manually inside your effect: that's a bad practice, too! With this code, you don't have the level of control you'd have with flattening operators (eg. cancelling the previous request).

As a general guide, suppose you have this situation:

map(action => condition ? otherAction : null)

This obviously doesn't work because null is not a valid action.

So, change map to switchMap (or another flattening operator) and modify your code this way:

import { switchMap, of, EMPTY } from 'rxjs';

...

switchMap(action => condition ? of(otherAction) : EMPTY)

In your scenario you want to perform a request conditionally. It'd be something like this instead of your tap:


switchMap(condition => {
  if (!condition) {
    return EMPTY
  }
  
  return this.service.update(...).pipe(
    take(1),
    map(x => yourAction())
  )
})

Obviously you can use any other flattening operator instead of switchMap. With switchMap a concurrent request would cancel the previous one, with mergeMap they'd be handled in parallel, with concatMap they'd be queued, with exhaustMap the second one would have no effect if the previous call hasn't finished.

Upvotes: 5

Related Questions