NGRX Bad Practices

In my previous articles I have covered Angular bad practices  (twice) and RxJS best practices. After working for 6 months using NGRX and understanding all of its benefits, now it is time for me to share some practices and patterns I (and often, the whole community) have recognized as harmful or complicated. So let's start with a list of what not to do in NGRX

(Almost) never subscribe to the Store: use selectors

Take a look at this code.

@Component({
  template: `
    <span>{{name}}</span>
  `
})
export class ComponentWithStore implements OnInit {
  name = '';
  
  constructor(store: Store<AppState>) {}
  
  ngOnInit() {
    this.store.subscribe(state => this.name = state.name);
  }
}

It's horrible, isn't it? First of all, subscribing to the store means we would then need unsubscribing (not implemented in the example), which means additional hassle. Secondly, it means a bit of imperative code in our component. And last, but not the least irritating fact is that we are not utilizing NGRX to its full potential. Let's change the code to make sure it is better:

@Component({
  template: `
    <span>{{ name$ | async }}</span>
  `
})
export class ComponentWithStore {
  name$ = this.store.select(state => state.name);
  
  constructor(store: Store<AppState>) {}
}

This is obviously better. We not only got rid of the need to unsubscribe (async  pipe does that automatically for us), but can also easily use ChangeDetectionStrategy.OnPush for better performance, and as another bonus, have reduced the amount of code and made it more declarative. Here is another example:

@Component({
  template: `
    <span>{{name}}</span>
  `
})
export class ComponentWithStore implements OnInit {
  name = '';
  
  constructor(store: Store<AppState>) {}
  
  ngOnInit() {
    this.store.subscribe(state => {
      if (state.name === 'ReservedName') {
        this.store.dispatch(reservedNameEncountered());  
      }
    });
  }
}

In this example, we check if the state got some specific value and perform an action on it. This is a scenario that we might encounter in any application, but this is not the correct way to implement it: the consumers of the derived state should not react with actions on state changes; for that purpose, we have Effects:

export class Effects {
  reservedName$ = createEffect(() => this.actions$.pipe(
    ofType(actions.setName),
    filter(({payload}) => payload === 'ReservedName'),
    map(() => reservedNameEncountered())
  ));
  
  constructor(actions$: Actions) {}
}

And zero code in the component. Remember, only action should trigger state changes, and we should only react to those actions with Effects and Reducers, not through a derived state.

But the title of this section mentions "almost"... so apparently there are case when we can subscribe to the Store? Imagine this scenario: our application has a complex system of permissions regulated by an administrator that can change in real time. Of course, those permissions are stored in the AppState. Now imagine we use Reactive forms in a component, but some fields might become disabled if an administrator changes that while we are filling in the form, and our state will get updated in real time. So how do we disable an input based on a permission from the Store?

@Component({
  template: `
    omitted for brevity
  `
})
export class ComponentWithStore implements OnInit {
  permissions$ = this.store.select(state => state.permissions);
  form = this.formBuilder.group({
    firstName: ['', Validators.require],
  });
  
  constructor(store: Store<AppState>) {}
  
  ngOnInit() {
    this.permissions$.subscribe(permissions => {
      const control = this.form.get('firstName');
      if (permissions.canEditFirstName) {
        control.enable();
      } else {
        control.disable();
      }
    });
  }
}

This thing is, we only can disable a Reactive FormControl using its method disable, which is inherently imperative with no declarative alternative (if we have to use Reactive forms). In this case, because the disabled state of the control depends on a value from our AppState, so we are forced to subscribe to the Store. So here is a rule of thumb:

Never subscribe to the Store manually, unless you have to perform an imperative third-party function with no alternatives, like FormControl.disable. (and if you do, remember to unsubscribe!)

Another nice and relatively new way of handling such situations without subscribing is using @ngrx/component-store with its component effects.

Don't pipe the Observable selected from the store

Okay, we stopped subscribing to the store or derived state Observables, so it is all shiny now, right? Not so much. Take a look at this code:

@Component({
  template: `
    <span *ngFor="let user of (activeUsers$ | async)">{{ user.name }}</span>
  `
})
export class ComponentWithStore {
  activeUsers$ = this.store.select(state => state.users).pipe(
    map(users => users.filter(user => user.isActive)),
  );
  
  constructor(store: Store<AppState>) {}
}

In this example, the original state is stored as an Array inside our AppState, but in this component we actually need a list of only active users. So we just transform the Observable of the derived state using operators and pure functions, that is certainly not bad, right?

The thing is, even the simplest RxJS operators are more complicated that simple selector functions, and result is code noise, and also are more difficult to debug if our logic is complex. So we will solve this problem with a named selector:

// selectors.ts
const activeUsers = (state: AppState) => state.users.filter(user => user.isActive)

And then, we immediately use the resulting derived state:

@Component({
  template: `
    <span *ngFor="let user of (activeUsers$ | async)">{{ user.name }}</span>
  `
})
export class ComponentWithStore {
  activeUsers$ = this.store.select(activeUsers);
  
  constructor(store: Store<AppState>) {}
}

This is both less code, more readable, it is instantly understandable what is being selected, so it is also more declarative. So, the answer to this bad practice is NGRX selectors.

Don't pipe and use operators on derived state, instead, use named selectors

Don't use combineLatest. Use named selectors instead

Application states can be very complicated at times, and can depend on multiple state forms; sometimes, a derived state is not derived from just one original state, but is a combination of two or more original states. Here is an example, imagine we have items of clothing as an Array of Clothing type objects, and also a shopping cart, which is also an Array of Clothing type objects. We can add new Clothing-s using a button "Add to cart", but when the Clothing is already present in the cart, we need to show a different button saying "Remove from Cart". Now, the logic is not too complicated, we just need to add a property isInShoppingCart on every item of Clothing in our derived list of items, which shows whether that item's id is present in the shopping cart Array. We have two selectors that allow us to select all items and the shopping cart Array. Here is our component:

@Component({
  template: `
    <app-clothing-item
      *ngFor="let item of (clothingItems$ | async)" [item]="item">
    </app-clothing-item>
  `
})
export class ClothingItemListComponent {
  clothingItems$ = combineLatest([
    this.store.select(state => state.clothingItems),
    this.store.select(state => state.cart),
  ]).pipe(
    map(([items, cart]) => items.map(item => ({
      ...item,
      isInShoppingCart: cart.map(cartItem => cartItem.id).includes(item.id),
    })))
  );
  
  constructor(store: Store<AppState>) {}
}

Now, it is easy to see how this logic is a bit too complex to put inside a component class. Also, again, while it is declarative, it is not readily apparent what happens here until we actually read and understand the code in depth. So what can be done with this situation? The answer, again, is selectors. NGRX allows us to combine different selectors using createSelector:

const allItems = (state: AppState) => state.clothingItems;

const shoppingCart = (state: AppState) => state.shoppingCart;

const cartIds = createSelector(shoppingCart, cart => cart.map(item => item.id));

const clothingItems = createSelector(
  allItems,
  cartIds,
  (items, cart) => items.map(item => ({
    ...item,
    isInShoppingCart: cart.includes(item.id),
  }),
);

Now these functions are way easier to understand. First we select all items and the cart. Then we create another selector that only selects the ids of a cart (selectors are memoized). And at last, we combine the two selectors to transform our list of all items. In that particular component we will only use the resulting selector. You might ask, why do we then create four selectors? But the thing is, it is better to have more simple selectors than few complex ones. It provides for great reusability and composability.

Whenever you notice combineLatest used in couple with derived state, think of combining selectors using createSelector

Try to avoid modifying nested state using withLatestFrom

Sometimes performing Effects requires us to take into consideration some other state that already exists in the store. For example: imagine we have a table which can be filtered and sorted using some buttons. Now we have a setSorting action that takes a sorting object ({field: string, direction: -1 | 1}) and adds it to a query object that contains all the query informations (filters, pagination and sorting, we have separate actions for three of them), and then sends the resulting object to the backend using a service. Notice the backend only accepts the entire query (all the sorting, filtering and pagination), rather than separate entities, but our action only modifies the sorting portion (modifying nested state). And we might not have the full query in the component from which we dispatch the setSorting action, which might tempt us to do the following in our effect:

@Injectable()
export class Effects {

  getData$ = createEffect(() => this.actions$.pipe(
    ofType(setSorting, setFilters, setPagination),
    withLatestFrom(this.store.select(state => state.query)),
    map(([{payload}, query]) => ({...query, [payload.type]: payload.data})),
    exhaustMap(query => this.dataService.getData(query).pipe(
      map(response => getDataSuccess(response)),
      catchError(error => of(getDataError(error)))
    )),
  ));

  constructor(
    private readonly actions$: Actions,
    private readonly store: Store<AppState>,
    private readonly dataService: DataService,
  ) {}

}

Now there are lots of negative things that we have done here in a desperate attempt to avoid doing some work in the component. First of all notice we now handle three actions that send the same request (one for sorting, one for filtering, and one for pagination), and we also take some already existing state using withLatestFrom. Now what can be done with this? First of all, let's get rid of three separate actions (setSorting, setFilters and setPagination). As Mike Ryan explains in this video, it is very important to keep our actions clean and concise. We now only have a single action getData that accepts the entire query object as payload. From our component, we shall do the following:

@Component({
  template: `
    <ng-container *ngIf="query$ | async as query">
      <app-sorting (sort)="setSorting($event, query)"></app-sorting>
      <app-filters (filter)="setFilters($event, query)"></app-filters>
      <app-table-data [data]="data$ | async"></app-table-data>
      <app-pagination (paginate)="setPagination($event, query)"></app-pagination>
    </ng-container>
  `,
})
export class TablePresenterComponent {

  query$ = this.store.select(state => state.query);
  data$ = this.store.select(state => state.data);

  constructor(
    private readonly store: Store<AppState>,
  ) {}
  
  setSorting(sorting: Sorting, query: Query) {
    this.store.dispatch(getData({...query, sorting}));
  }
  
  setFilters(sorting: Sorting, filters: Filters) {
    this.store.dispatch(getData({...query, filters}));
  }
  
  setPagination(sorting: Sorting, pagination: Pagination) {
    this.store.dispatch(getData({...query, pagination}));
  }

} 

Notice how our component is still simple and concise, every method handles different scenarios while dispatching the same action. And because the action is the same, we can now change our effect to look like this:

@Injectable()
export class Effects {

  getTableData$ = createEffect(() => this.actions$.pipe(
    ofType(getData),
    exhaustMap(({payload}) => this.dataService.getData(payload).pipe(
      map(response => getDataSuccess(response)),
      catchError(error => of(getDataError(error)))
    )),
  ));

  constructor(
    private readonly actions$: Actions,
    private readonly dataService: DataService,
  ) {}

}

Now we only handle one action, we only do one simple thing, and we got rid of withLatestFrom.

withLatestFrom inside of an effect might be a code smell, which indicates something can probably be done in a simpler way.

Never keep derived state inside the store

One of the most important aspects of dealing with NGRX (and any state management system for that matter) is differentiating between original and derived state. Original state is what we actually store in the AppState. For example, data that is received from the backend and immediately put into the store is the original state. Derived state is a form of state created from the original state via some transformation, usually through a selector. For instance, the list of Clothing items from the example about combining selectors is a clear example of derived state.

Now sometimes developers are tempted to put the derived state alongside the original state inside the store. That is a bad practice for a number of reasons:

1. We store more data in the store that is actually needed
2. It becomes necessary to sync two pieces of state, if some action modifies the original state, we now also have to modify the derived state
3. AppState looks very messy

Take a look at this example:

const _reducer = createReducer(
  initialState,
  on(clothingActions.filter, (state, {payload}) => ({
    ...state,
    filteredClothings: state.clothings.filter(
      clothing => clothing.name.includes(query),
    ),
  })),
);

Now here we have both original state (the list of all clothings) and a derived state (filtered list). But it would be better if we stored the query and derived the filtered clothings using a combined selector of the query and the entire list. Here it is:

// reducer.ts

const _reducer = createReducer(
  initialState,
  on(clothingActions.filter, (state, {payload}) => ({
    ...state,
    query: payload,
  })),
);

// selectors.ts

const allClothings = (state: AppState) => state.clothings;

const query = (state: AppState) => state.query;

const filteredClothings = createSelector(
  allClothings,
  query,
  (clothings, query) => clothing.filter(
    clothing => clothing.name.includes(query),
  ),
);

And then we can simply use the derived state selector in our component.

Conclusion

You might have noticed that the answer to many bad practices is using selectors. This is because the main problem of state management systems is the differentiation of derived state and original state. When we manage to successfully decide which one is which, and create concise ways of manipulating and selecting those, we will have a simple, declarative and reactive Angular app built on NGRX.