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

selectSignal() fires on every global state change #3882

Closed
1 of 2 tasks
mikezks opened this issue May 8, 2023 · 9 comments · Fixed by #3883
Closed
1 of 2 tasks

selectSignal() fires on every global state change #3882

mikezks opened this issue May 8, 2023 · 9 comments · Fixed by #3883
Labels
bug community watch Someone from the community is working this issue/PR Project: Store

Comments

@mikezks
Copy link
Contributor

mikezks commented May 8, 2023

Which @ngrx/* package(s) are the source of the bug?

store

Minimal reproduction of the bug/regression with instructions

All selectors created with selectSignal() fire on every global state change, even if the related state received no update.

store = inject(Store);

constructor() {
   const myState = this.store.selectSignal(mySelector);
   effect(() => console.log(myState());
   setTimeout(() => {
      this.store.dispatch(
         changeOtherStateAction({ info: 'globale state changed' })
      );
   },  5_000);
}

Why does this happen?

The Angular team decided to emit a new Signal value for each object set, even if the reference stays the same.
This is an issue for working with immuable data (the default for NgRx state management).

The behavior can be fixes manually by configuring an equal function:

this.store.selectSignal(
   mySelector,
   { equal: (a, b) => a === b }
);

Necessary change

This equal compare behavior should be set automatically for selectSignal() as the current behavior is not expected by the developer that works with an immutable state pattern.

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

NgRx: 16 RC.0
Angular 16.0.0

Other information

May affect @ngrx/component-store as well.

Credits to @manfredsteyer who found this issue.

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

  • Yes
  • No
@mikezks
Copy link
Contributor Author

mikezks commented May 8, 2023

@markostanimirovic
Copy link
Member

markostanimirovic commented May 8, 2023

Hey @mikezks,

I'd like to see how mySelector implementation looks like. I tried to reproduce the issue, but it seems that everything works fine. Here is the playground that you can check: https:/marko-jobcloud/ngrx-platform/blob/select-signal-potential-issue/projects/standalone-app/src/app/app.component.ts#L36

Let me know if I'm missing something.


EDIT: I reproduced it by changing count1 property from the example to be an object instead of primitive. Interesting! 😅

@mikezks do you want to create a PR to fix this issue?

It can be fixed by changing the Store.selectSignal method as follows:

  selectSignal<K>(
    selector: (state: T) => K,
    options?: SelectSignalOptions<K>
  ): Signal<K> {
    return computed(() => selector(this.state()), {
      equal: options?.equal || ((previous, current) => previous === current),
    });
  }

@mikezks
Copy link
Contributor Author

mikezks commented May 8, 2023

Thanks for checking that, @markostanimirovic.

Yes, I will provide a PR for that.

@markostanimirovic markostanimirovic added the community watch Someone from the community is working this issue/PR label May 8, 2023
mikezks added a commit to mikezks/ngrx-platform that referenced this issue May 8, 2023
@wSedlacek
Copy link

I noticed that my this.store.select() observables were emitting 3 times on 16.0.0-rc0.
Removing the toSignal() caused that behavior to go away.
Perhaps the changes already made also fixed that issue? I'll test again in the next rc.

@mikezks
Copy link
Contributor Author

mikezks commented May 8, 2023

I noticed that my this.store.select() observables were emitting 3 times on 16.0.0-rc0. Removing the toSignal() caused that behavior to go away. Perhaps the changes already made also fixed that issue? I'll test again in the next rc.

I assume this is not related.
Nevertheless it is worth analyzing that behavior. Could you provide a repo to reproduce that?

@mikezks
Copy link
Contributor Author

mikezks commented May 8, 2023

More context:
toSignal() has the discussed behavior as well that same object references would re-emit, but the underlying Observable should not emit a new value at all.

@wSedlacek
Copy link

wSedlacek commented May 8, 2023

So in a trivial app the behavior I observed does not occur. (The tap() is only called once in the console)
https://stackblitz.com/edit/angular-erbuhr?file=src/main.ts

The app I noticed the issue on has some more complexities which I have not fully investigated in the reproduction though.
Specifically:

  1. Lazy loaded features
  2. Effects that use this.state.select() as a source
  3. Nx buildable libraries (with multiple entry points)
  4. Observables subscribed in services provided by components
  5. this.store.select() used in router guards
  6. @ngrx/router-store
  7. Nested router outlets with lazy loaded modules

There are probably a lot of other things in the app I tested with that could impact this beyond even just these. I will let you know out if I find any specific situations that recreate the behavior.

@mikezks
Copy link
Contributor Author

mikezks commented May 8, 2023

Great, thanks. If the regression is specific to the v16-next update or later v16-final, this is definitely important to know.

@wSedlacek
Copy link

Just tested in 16.0.0 and the issue I observed does not occur. 🎉
Thank you for all the bug fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug community watch Someone from the community is working this issue/PR Project: Store
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants