feat(entity): log a warning message when selectId returns undefined in dev mode (#1169)

Closes #1133

Author: timdeschryver

modules/entity/spec/utils.spec.ts

@@ -0,0 +1,57 @@
+import * as ngCore from '@angular/core';
+import { selectIdValue } from '../src/utils';
+import { BookModel, AClockworkOrange } from './fixtures/book';
+
+describe('Entity utils', () => {
+  describe(`selectIdValue()`, () => {
+    it('should not warn when key does exist', () => {
+      const spy = spyOn(console, 'warn');
+
+      const key = selectIdValue(AClockworkOrange, book => book.id);
+
+      expect(spy).not.toHaveBeenCalled();
+    });
+
+    it('should warn when key does not exist in dev mode', () => {
+      const spy = spyOn(console, 'warn');
+
+      const key = selectIdValue(AClockworkOrange, (book: any) => book.foo);
+
+      expect(spy).toHaveBeenCalled();
+    });
+
+    it('should warn when key is undefined in dev mode', () => {
+      const spy = spyOn(console, 'warn');
+
+      const undefinedAClockworkOrange = { ...AClockworkOrange, id: undefined };
+      const key = selectIdValue(
+        undefinedAClockworkOrange,
+        (book: any) => book.id
+      );
+
+      expect(spy).toHaveBeenCalled();
+    });
+
+    it('should not warn when key does not exist in prod mode', () => {
+      spyOn(ngCore, 'isDevMode').and.returnValue(false);
+      const spy = spyOn(console, 'warn');
+
+      const key = selectIdValue(AClockworkOrange, (book: any) => book.foo);
+
+      expect(spy).not.toHaveBeenCalled();
+    });
+
+    it('should not warn when key is undefined in prod mode', () => {
+      spyOn(ngCore, 'isDevMode').and.returnValue(false);
+      const spy = spyOn(console, 'warn');
+
+      const undefinedAClockworkOrange = { ...AClockworkOrange, id: undefined };
+      const key = selectIdValue(
+        undefinedAClockworkOrange,
+        (book: any) => book.id
+      );
+
+      expect(spy).not.toHaveBeenCalled();
+    });
+  });
+});

modules/entity/src/sorted_state_adapter.ts

@@ -8,6 +8,7 @@ import {
 } from './models';
 import { createStateOperator, DidMutate } from './state_adapter';
 import { createUnsortedStateAdapter } from './unsorted_state_adapter';
+import { selectIdValue } from './utils';
 
 export function createSortedStateAdapter<T>(
   selectId: IdSelector<T>,
@@ -28,7 +29,7 @@ export function createSortedStateAdapter<T>(selectId: any, sort: any): any {
   function addManyMutably(newModels: T[], state: R): DidMutate;
   function addManyMutably(newModels: any[], state: any): DidMutate {
     const models = newModels.filter(
-      model => !(selectId(model) in state.entities)
+      model => !(selectIdValue(model, selectId) in state.entities)
     );
 
     if (models.length === 0) {
@@ -62,7 +63,7 @@ export function createSortedStateAdapter<T>(selectId: any, sort: any): any {
 
     const original = state.entities[update.id];
     const updated = Object.assign({}, original, update.changes);
-    const newKey = selectId(updated);
+    const newKey = selectIdValue(updated, selectId);
 
     delete state.entities[update.id];
 
@@ -117,7 +118,7 @@ export function createSortedStateAdapter<T>(selectId: any, sort: any): any {
     const updated: any[] = [];
 
     for (const entity of entities) {
-      const id = selectId(entity);
+      const id = selectIdValue(entity, selectId);
       if (id in state.entities) {
         updated.push({ id, changes: entity });
       } else {
@@ -151,7 +152,7 @@ export function createSortedStateAdapter<T>(selectId: any, sort: any): any {
 
     while (i < models.length && j < state.ids.length) {
       const model = models[i];
-      const modelId = selectId(model);
+      const modelId = selectIdValue(model, selectId);
       const entityId = state.ids[j];
       const entity = state.entities[entityId];
 

modules/entity/src/unsorted_state_adapter.ts

@@ -1,5 +1,6 @@
 import { EntityState, EntityStateAdapter, IdSelector, Update } from './models';
 import { createStateOperator, DidMutate } from './state_adapter';
+import { selectIdValue } from './utils';
 
 export function createUnsortedStateAdapter<T>(
   selectId: IdSelector<T>
@@ -9,7 +10,7 @@ export function createUnsortedStateAdapter<T>(selectId: IdSelector<T>): any {
 
   function addOneMutably(entity: T, state: R): DidMutate;
   function addOneMutably(entity: any, state: any): DidMutate {
-    const key = selectId(entity);
+    const key = selectIdValue(entity, selectId);
 
     if (key in state.entities) {
       return DidMutate.None;
@@ -81,7 +82,7 @@ export function createUnsortedStateAdapter<T>(selectId: IdSelector<T>): any {
   ): boolean {
     const original = state.entities[update.id];
     const updated: T = Object.assign({}, original, update.changes);
-    const newKey = selectId(updated);
+    const newKey = selectIdValue(updated, selectId);
     const hasNewKey = newKey !== update.id;
 
     if (hasNewKey) {
@@ -133,7 +134,7 @@ export function createUnsortedStateAdapter<T>(selectId: IdSelector<T>): any {
     const updated: any[] = [];
 
     for (const entity of entities) {
-      const id = selectId(entity);
+      const id = selectIdValue(entity, selectId);
       if (id in state.entities) {
         updated.push({ id, changes: entity });
       } else {

modules/entity/src/utils.ts

@@ -0,0 +1,19 @@
+import { isDevMode } from '@angular/core';
+import { IdSelector } from './models';
+
+export function selectIdValue<T>(entity: T, selectId: IdSelector<T>) {
+  const key = selectId(entity);
+
+  if (isDevMode() && key === undefined) {
+    console.warn(
+      '@ngrx/entity: The entity passed to the `selectId` implementation returned undefined.',
+      'You should probably provide your own `selectId` implementation.',
+      'The entity that was passed:',
+      entity,
+      'The `selectId` implementation:',
+      selectId.toString()
+    );
+  }
+
+  return key;
+}