fix(store): improve consistency of memoized selector result when proj…

…ection fails (#2101) A memoized selector projection now consistently fails after the first evaluation, if it is evaluated again with the same state, instead of returning that last successful evaluation result from a previous state. Closes #2100
ngrx · Sep 7, 2019 · c63941c · c63941c
1 parent 70a8f2d
commit c63941c
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 82 deletions.
diff --git a/modules/store/spec/selector.spec.ts b/modules/store/spec/selector.spec.ts
@@ -41,23 +41,17 @@ 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);
     });
 
     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);
 
@@ -70,11 +64,7 @@ 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('', '');
 
@@ -92,10 +82,7 @@ describe('Selectors', () => {
           return state.unchanged;
         });
       const projectFn = jasmine.createSpy('projectionFn');
-      const selector = createSelector(
-        neverChangingSelector,
-        projectFn
-      );
+      const selector = createSelector(neverChangingSelector, projectFn);
 
       selector(firstState);
       selector(secondState);
@@ -126,13 +113,33 @@ describe('Selectors', () => {
       expect(projectFn).toHaveBeenCalledTimes(2);
     });
 
+    it('should not memoize last successful projection result in case of error', () => {
+      const firstState = { ok: true };
+      const secondState = { ok: false };
+      const fail = () => {
+        throw new Error();
+      };
+      const projectorFn = jasmine
+        .createSpy('projectorFn', (s: any) => (s.ok ? s.ok : fail()))
+        .and.callThrough();
+      const selectorFn = jasmine
+        .createSpy('selectorFn', createSelector(state => state, projectorFn))
+        .and.callThrough();
+
+      selectorFn(firstState);
+
+      expect(() => selectorFn(secondState)).toThrow(new Error());
+      expect(() => selectorFn(secondState)).toThrow(new Error());
+
+      selectorFn(firstState);
+      expect(selectorFn).toHaveBeenCalledTimes(4);
+      expect(projectorFn).toHaveBeenCalledTimes(3);
+    });
+
     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);
@@ -144,18 +151,9 @@ 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();
 
@@ -271,20 +269,16 @@ 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('', '');
 
@@ -302,10 +296,7 @@ describe('Selectors', () => {
           return state.unchanged;
         });
       const projectFn = jasmine.createSpy('projectionFn');
-      const selector = createSelector(
-        [neverChangingSelector],
-        projectFn
-      );
+      const selector = createSelector([neverChangingSelector], projectFn);
 
       selector(firstState);
       selector(secondState);
@@ -337,10 +328,7 @@ 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);
@@ -352,18 +340,9 @@ 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();
 

diff --git a/modules/store/src/selector.ts b/modules/store/src/selector.ts
@@ -95,9 +95,9 @@ export function defaultMemoize(
       return lastResult;
     }
 
+    const newResult = projectionFn.apply(null, arguments as any);
     lastArguments = arguments;
 
-    const newResult = projectionFn.apply(null, arguments as any);
     if (isResultEqual(lastResult, newResult)) {
       return lastResult;
     }
@@ -603,22 +603,19 @@ export function createFeatureSelector<T, V>(
 export function createFeatureSelector(
   featureName: any
 ): MemoizedSelector<any, any> {
-  return createSelector(
-    (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
-  );
+  return createSelector((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);
 }