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

Selector function runs multiple times even though arguments have not changed #1389

Closed
Abrissirba opened this issue Oct 30, 2018 · 2 comments · Fixed by #1393
Closed

Selector function runs multiple times even though arguments have not changed #1389

Abrissirba opened this issue Oct 30, 2018 · 2 comments · Fixed by #1393

Comments

@Abrissirba
Copy link

Minimal reproduction of the bug/regression with instructions:

https://stackblitz.com/edit/ngrx-seed-vnhhsk?file=src%2Fapp%2Freducers%2Findex.ts

Setup:

  • The example contains of a store with a state that constains two properties. One entity object that contains objects with their id as key and a counter property.
  • There is a factory function to create a selector that returns an object from the store with the provided id. The entity selector does some computations, sleeps for 1 second in this example and we would therefore want it to be memoized so that the computation doesn't have to run too often.
  • the app.component is calling the factory function to get a selector for the entity with id 1.

Steps to reproduce:
1 Click on the + button to increase the counter. This will be quick and not trigger the getEntity selector
2 Click on the "Update 2" button. This will update the entity with id 2 and the selector function will be executed.
3 Click on the + button again. This time it will not be quick since the getEntity selector was triggered again. However, it shouldn't have because the entity object was not updated.

Expected behavior:

When clicking on the + button in step 3, the created getEntity selector should not be called since the entity object has not been changed.

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

6.1.0

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

I think the error is caused by the memoize function here: https://github.com/ngrx/platform/blob/master/modules/store/src/selector.ts#L62. If the arguments were changed but not the result, the lastArguments will never be reassigned. This means that the selector will be called the next time the memoize function is called since it thinks that the arguments has been changed. This can be prevented by adding the following line
lastArguments = arguments;
in the if(isResultEqual...

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

@sebastienlachance
Copy link

Same behaviour here. All our selectors are created via createSelectorFactory with a custom memoization function to bypass the result check. But sometime the defaultMemoize is called anyway.

@timdeschryver
Copy link
Member

timdeschryver commented Oct 31, 2018

Sorry I didn't notice you came to the same conclusion as I did, after I created the PR 😅 .
But it rang a bell from a previous commit so I went ahead and created the PR.

brandonroberts pushed a commit that referenced this issue Nov 1, 2018
The arguments weren't being memoized when the result of the selector was equal to its previous result. This fix moves the memoization of the arguments before the check if the results are equal.

Closes #1389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants