Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memoized selectors: Inconsistent behavior when projection fails #2100

Closed
REPLicated opened this issue Aug 31, 2019 · 0 comments · Fixed by #2101
Closed

Memoized selectors: Inconsistent behavior when projection fails #2100

REPLicated opened this issue Aug 31, 2019 · 0 comments · Fixed by #2101

Comments

@REPLicated
Copy link
Contributor

Description

The current behavior of memoization as implemented in defaultMemoize
can be very surprising in the case of a failed evaluation of the projection.

An error may be thrown in a projector either due to a bug in the projector logic itself (unanticipated cases when the projector is very complex) or when a precondition for the projector is not fulfilled (i.e. inconsistent store state due to a bug elsewhere).

Let selector be any memoizing selector, GS a "good state" from which R is selected, and FS a "bad state" for which the projection fails. The current behavior for three consecutive calls to the selector is (pseudocode):

selector(GS) => returns R
selector(FS) => throws
selector(FS) => returns R

I would expect the third call to the selector to throw as well. There seems to be no good argument why R should suddenly be selected from FS.
Therefore I suggest a fix to the memoization logic such that:

selector(GS) => returns R
selector(FS) => throws
selector(FS) => throws

Minimal reproduction of the bug/regression with instructions:

The following minimal (currently failing) testcase reproduces the issue:

      const firstState = { ok: 'a' };
      const secondState = { faulty: 'b' };
      const fail = () => { throw new Error(); };
      const selector = createSelector(
        identity,
        (s: any) => s.ok ? s.ok : fail()
      );

      selector(firstState);

      expect(() => selector(secondState)).toThrow(new Error());
      expect(() => selector(secondState)).toThrow(new Error()); // fails here, returns 'a'

Expected behavior:

As described above, I would expect more consistent behavior when the selector is triggered a second time with the same "faulty" state.

In simple scenarios, the current behavior may often be overlooked because the selector is usually not evaluated again with the same state (due to distinctUntilChanged in the store). However, if the selector is nested and only a part of the state changes it may happen of course. Further, if a select fails due to an error in a selector, the observable usually errors out. But when chained with operators like retry, the current behavior can obfuscate problems and may be very hard to debug.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

Current NgRx master

Other information:

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

REPLicated added a commit to REPLicated/platform that referenced this issue Aug 31, 2019
REPLicated added a commit to REPLicated/platform that referenced this issue Aug 31, 2019
REPLicated added a commit to REPLicated/platform that referenced this issue Aug 31, 2019
…ection fails

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 ngrx#2100
REPLicated added a commit to REPLicated/platform that referenced this issue Aug 31, 2019
…ection fails

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 ngrx#2100
brandonroberts pushed a commit that referenced this issue Sep 7, 2019
…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
jordanpowell88 pushed a commit to jordanpowell88/platform that referenced this issue Nov 14, 2019
…ection fails (ngrx#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 ngrx#2100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants