From 6946e2eb97c32440eb3923fac031b6b639069a8b Mon Sep 17 00:00:00 2001 From: John Crowson Date: Mon, 26 Aug 2019 20:45:21 -0600 Subject: [PATCH] feat(store): add verbose error message for undefined feature state in development mode (#2078) Closes #1897 --- modules/store/spec/selector.spec.ts | 129 ++++++++++++++++++++++++---- modules/store/src/selector.ts | 18 +++- 2 files changed, 127 insertions(+), 20 deletions(-) diff --git a/modules/store/spec/selector.spec.ts b/modules/store/spec/selector.spec.ts index 4f24b2f7a4..368a4ed187 100644 --- a/modules/store/spec/selector.spec.ts +++ b/modules/store/spec/selector.spec.ts @@ -1,3 +1,4 @@ +import * as ngCore from '@angular/core'; import { cold } from 'jasmine-marbles'; import { createSelector, @@ -40,9 +41,11 @@ describe('Selectors', () => { it('should deliver the value of selectors to the projection function', () => { const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector(incrementOne, incrementTwo, projectFn)( - {} - ); + const selector = createSelector( + incrementOne, + incrementTwo, + projectFn + )({}); expect(projectFn).toHaveBeenCalledWith(countOne, countTwo); }); @@ -50,7 +53,11 @@ describe('Selectors', () => { it('should allow an override of the selector return', () => { const projectFn = jasmine.createSpy('projectionFn').and.returnValue(2); - const selector = createSelector(incrementOne, incrementTwo, projectFn); + const selector = createSelector( + incrementOne, + incrementTwo, + projectFn + ); expect(selector.projector()).toBe(2); @@ -63,7 +70,11 @@ describe('Selectors', () => { it('should be possible to test a projector fn independent from the selectors it is composed of', () => { const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector(incrementOne, incrementTwo, projectFn); + const selector = createSelector( + incrementOne, + incrementTwo, + projectFn + ); selector.projector('', ''); @@ -81,7 +92,10 @@ describe('Selectors', () => { return state.unchanged; }); const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector(neverChangingSelector, projectFn); + const selector = createSelector( + neverChangingSelector, + projectFn + ); selector(firstState); selector(secondState); @@ -115,7 +129,10 @@ describe('Selectors', () => { it('should allow you to release memoized arguments', () => { const state = { first: 'state' }; const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector(incrementOne, projectFn); + const selector = createSelector( + incrementOne, + projectFn + ); selector(state); selector(state); @@ -127,9 +144,18 @@ describe('Selectors', () => { }); it('should recursively release ancestor selectors', () => { - const grandparent = createSelector(incrementOne, a => a); - const parent = createSelector(grandparent, a => a); - const child = createSelector(parent, a => a); + const grandparent = createSelector( + incrementOne, + a => a + ); + const parent = createSelector( + grandparent, + a => a + ); + const child = createSelector( + parent, + a => a + ); spyOn(grandparent, 'release').and.callThrough(); spyOn(parent, 'release').and.callThrough(); @@ -245,16 +271,20 @@ describe('Selectors', () => { describe('createSelector with arrays', () => { it('should deliver the value of selectors to the projection function', () => { const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector([incrementOne, incrementTwo], projectFn)( - {} - ); + const selector = createSelector( + [incrementOne, incrementTwo], + projectFn + )({}); expect(projectFn).toHaveBeenCalledWith(countOne, countTwo); }); it('should be possible to test a projector fn independent from the selectors it is composed of', () => { const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector([incrementOne, incrementTwo], projectFn); + const selector = createSelector( + [incrementOne, incrementTwo], + projectFn + ); selector.projector('', ''); @@ -272,7 +302,10 @@ describe('Selectors', () => { return state.unchanged; }); const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector([neverChangingSelector], projectFn); + const selector = createSelector( + [neverChangingSelector], + projectFn + ); selector(firstState); selector(secondState); @@ -304,7 +337,10 @@ describe('Selectors', () => { it('should allow you to release memoized arguments', () => { const state = { first: 'state' }; const projectFn = jasmine.createSpy('projectionFn'); - const selector = createSelector([incrementOne], projectFn); + const selector = createSelector( + [incrementOne], + projectFn + ); selector(state); selector(state); @@ -316,9 +352,18 @@ describe('Selectors', () => { }); it('should recursively release ancestor selectors', () => { - const grandparent = createSelector([incrementOne], a => a); - const parent = createSelector([grandparent], a => a); - const child = createSelector([parent], a => a); + const grandparent = createSelector( + [incrementOne], + a => a + ); + const parent = createSelector( + [grandparent], + a => a + ); + const child = createSelector( + [parent], + a => a + ); spyOn(grandparent, 'release').and.callThrough(); spyOn(parent, 'release').and.callThrough(); @@ -433,9 +478,11 @@ describe('Selectors', () => { describe('createFeatureSelector', () => { let featureName = '@ngrx/router-store'; let featureSelector: (state: any) => number; + let warnSpy: jasmine.Spy; beforeEach(() => { featureSelector = createFeatureSelector(featureName); + warnSpy = spyOn(console, 'warn'); }); it('should memoize the result', () => { @@ -455,6 +502,50 @@ describe('Selectors', () => { ); expect(featureState$).toBeObservable(expected$); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('should warn if the feature does not exist in the state', () => { + spyOn(ngCore, 'isDevMode').and.returnValue(true); + + const state = { otherState: '' }; + + const state$ = cold('a', { a: state }); + const expected$ = cold('a', { a: undefined }); + + const featureState$ = state$.pipe( + map(featureSelector), + distinctUntilChanged() + ); + + expect(featureState$).toBeObservable(expected$); + expect(warnSpy).toHaveBeenCalledWith( + 'The feature name "@ngrx/router-store" does not exist ' + + 'in the state, therefore createFeatureSelector cannot ' + + 'access it. Be sure it is imported in a loaded module using ' + + "StoreModule.forRoot('@ngrx/router-store', ...) or " + + "StoreModule.forFeature('@ngrx/router-store', ...). If the " + + 'default state is intended to be undefined, as is the case ' + + 'with router state, this development-only warning message can ' + + 'be ignored.' + ); + }); + + it('should not warn if not in development mode', () => { + spyOn(ngCore, 'isDevMode').and.returnValue(false); + + const state = { otherState: '' }; + + const state$ = cold('a', { a: state }); + const expected$ = cold('a', { a: undefined }); + + const featureState$ = state$.pipe( + map(featureSelector), + distinctUntilChanged() + ); + + expect(featureState$).toBeObservable(expected$); + expect(warnSpy).not.toHaveBeenCalled(); }); }); diff --git a/modules/store/src/selector.ts b/modules/store/src/selector.ts index 3600cd78a9..5de9eee060 100644 --- a/modules/store/src/selector.ts +++ b/modules/store/src/selector.ts @@ -1,4 +1,5 @@ import { Selector, SelectorWithProps } from './models'; +import { isDevMode } from '@angular/core'; export type AnyFn = (...args: any[]) => any; @@ -274,6 +275,7 @@ export function createSelector( Selector, Selector, Selector, + Selector, Selector, Selector @@ -602,7 +604,21 @@ export function createFeatureSelector( featureName: any ): MemoizedSelector { return createSelector( - (state: any) => state[featureName], + (state: any) => { + const featureState = state[featureName]; + if (isDevMode() && featureState === undefined) { + console.warn( + `The feature name \"${featureName}\" does ` + + 'not exist in the state, therefore createFeatureSelector ' + + 'cannot access it. Be sure it is imported in a loaded module ' + + `using StoreModule.forRoot('${featureName}', ...) or ` + + `StoreModule.forFeature('${featureName}', ...). If the default ` + + 'state is intended to be undefined, as is the case with router ' + + 'state, this development-only warning message can be ignored.' + ); + } + return featureState; + }, (featureState: any) => featureState ); }