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

[signals] Prevent overriding state fields with a computed signal of the same name #4144

Open
1 of 2 tasks
jits opened this issue Nov 21, 2023 · 3 comments
Open
1 of 2 tasks

Comments

@jits
Copy link
Contributor

jits commented Nov 21, 2023

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

signals

Minimal reproduction of the bug/regression with instructions

I'm not sure if this is intentional or not: it's possible to define a computed field with the same name as a field already defined in the initial state.

Reproduction:

https://stackblitz.com/edit/stackblitz-starters-atpbi6?file=src%2Fmain.ts

For posterity, here's the store definition used in the StackBlitz above:

const Store = signalStore(
  { providedIn: 'root' },
  withState({ value: 1 }),
  withComputed((store) => ({
    value: computed(() => store.value() * 2),
  }))
);

This may lead to unintentional bugs.

Expected behavior

You should get an error if you try to define a computed field with same name as an existing field in the initial state.

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

Angular: v17
NgRx: v17.0.0

Other information

No response

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

  • Yes
  • No
@markostanimirovic
Copy link
Member

Overriding behavior is by initial design - it's not a bug. On the other hand, it can be hard to catch bugs caused by unintentional overriding.

Btw, we already discussed introducing this change at the previous team meeting. 👀

@jits
Copy link
Contributor Author

jits commented Nov 21, 2023

Ah, that's good to know!

Not a big deal — could be covered by documentation (as a feature + gotcha).

Another thought: could log a warning in console when in dev mode.

@rainerhahnekamp
Copy link
Contributor

I have pushed a draft PR at #4199

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

No branches or pull requests

3 participants