platform icon indicating copy to clipboard operation
platform copied to clipboard

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

Open jits opened this issue 1 year ago • 3 comments

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

  • [X] Yes
  • [ ] No

jits avatar Nov 21 '23 11:11 jits

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. 👀

markostanimirovic avatar Nov 21 '23 15:11 markostanimirovic

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.

jits avatar Nov 21 '23 15:11 jits

I have pushed a draft PR at https://github.com/ngrx/platform/pull/4199

rainerhahnekamp avatar Jan 03 '24 18:01 rainerhahnekamp