platform icon indicating copy to clipboard operation
platform copied to clipboard

[signals] Potential typing bug when using discriminated unions

Open jits opened this issue 1 year ago • 9 comments

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

signals

Minimal reproduction of the bug/regression with instructions

See https://stackblitz.com/edit/stackblitz-starters-ucjue4?file=src%2Fstore.ts

For posterity, here is the code that causes the typing error:

import { Injectable } from '@angular/core';
import { signalStore, withState } from '@ngrx/signals';

type A = {
  status: 'A';
  thing: null;
};

type B = {
  status: 'B';
  thing: string;
};

type State = A | B;

const initialState: State = {
  status: 'A',
  thing: null,
};

const _Store = signalStore(
  { providedIn: 'root' },
  withState<State>(initialState)
);

@Injectable({ providedIn: 'root' })
export class Store extends _Store {}    // <-- Typing error occurs here

The typing error occurs on the line export class Store extends _Store {}, with the following TypeScript error:

Base constructor return type '({ status: Signal<"A">; thing: Signal<null>; } | { status: Signal<"B">; thing: Signal<string>; }) & StateSource<{ status: "A"; thing: null; } | { status: "B"; thing: string; }>' is not an object type or intersection of object types with statically known members.(2509)

Expected behavior

Should compile fine and not give any TypeScript errors.

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

NgRx: v18.0.0 Angular: latest v18 TypeScript: v5.5

Other information

No response

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

  • [X] Yes
  • [ ] No

jits avatar Jul 30 '24 09:07 jits

The error goes away if you just have

const _Store = signalStore(
  { providedIn: 'root' },
  withState(initialState)
);

initialState has already a declared type, so no need to provide the type a second time. Not sure if that helps in the long run, though.

rainerhahnekamp avatar Jul 30 '24 10:07 rainerhahnekamp

Thanks @rainerhahnekamp! That works for now (as a workaround) 👍🏽

jits avatar Jul 30 '24 10:07 jits

@markostanimirovic, if that's the right approach for now, should we leave a note in the docs?

rainerhahnekamp avatar Jul 30 '24 11:07 rainerhahnekamp

@rainerhahnekamp — I spoke too soon — unfortunately it still fails elsewhere, when using the discriminated union types. I've updated the Stackblitz to show this (https://stackblitz.com/edit/stackblitz-starters-ucjue4?file=src%2Fstore.ts).

Looks like the inferred state type of the store is not picking up the full discriminated union, just the specific type of initialState.

jits avatar Jul 30 '24 12:07 jits

Union types can be used for state properties, not for the entire state because SignalStore/SignalState generates state signals based on the initial state.

// From:
type State = A | B;
const initialState: State = { /* ... */ };

// To:
type State = { prop: A | B };
const initialState: State = { prop: /* ... */ };

We can add a section about unions to the FAQ page and update the answer for the "Can I define my SignalStore as a class?" question to contain a note that although it's possible to create a class-based SignalStore, using the functional style is recommended.

markostanimirovic avatar Jul 30 '24 12:07 markostanimirovic

@markostanimirovic — ahh, that's a shame! I've been using discriminated union types (with extended class) up until (and including) RC2. Is there anything that can be done to support this use case, as I feel it's more akin to modelling state like a state machine, and gives us much better type safety. I'm keen to use this with extended classes so I can add extra properties etc. (as mentioned in https://github.com/ngrx/platform/discussions/4141#discussioncomment-7629034).

A concrete example of where I'm doing this (an AuthStore):

jits avatar Jul 30 '24 12:07 jits

@jits. nested state with union works. We can take that as workaround

type A = {
  status: 'A';
  thing: null;
};

type B = {
  status: 'B';
  thing: string;
};

type NestedState = A | B;

type State = { nested: NestedState };

const initialState: State = { nested: { status: 'A', thing: null } };

const _Store = signalStore(
  { providedIn: 'root' },
  withState(initialState),
  withMethods((store) => {
    return {
      updateToB(thing: string): void {
        const newState: B = { status: 'B', thing };
        patchState(store, { nested: newState });
      },
    };
  })
);

rainerhahnekamp avatar Jul 30 '24 12:07 rainerhahnekamp

@markostanimirovic — ahh, that's a shame! I've been using discriminated union types (with extended class) up until (and including) RC2. Is there anything that can be done to support this use case, as I feel it's more akin to modelling state like a state machine, and gives us much better type safety. I'm keen to use this with extended classes so I can add extra properties etc. (as mentioned in #4141 (reply in thread)).

A concrete example of where I'm doing this (an AuthStore):

Thank you for the detailed explanation @jits!

We'll consider adding a withProps base feature to add properties to the SignalStore in the next major release. With that feature, I guess class-based stores won't be needed anymore.

In the meantime, I suggest a different approach. Instead of extending a SignalStore, you can create a class that injects it:

const AuthStore = signalStore({ providedIn: 'root' }, /* ... */);

@Injectable({ providedIn: 'root' })
export class AuthFacade {
  readonly #store = inject(AuthStore);

 // ... expose all public store APIs
 // ... add additional properties
}

By the way, discriminated unions for the entire state work with functional SignalStores: https://stackblitz.com/edit/stackblitz-starters-hefo5v?file=src%2Fstore.ts

markostanimirovic avatar Jul 30 '24 14:07 markostanimirovic

Thanks for clarifying @markostanimirovic, and for your suggestions.

Sounds like functional SignalStores are the way forward. A future withProps base feature would be handy, but a wrapper / helper class will work for now.

Congrats to you and the team on the NgRx SignalStore first stable release! 🎉

(Please feel free to close this issue if no further action is needed.)

jits avatar Jul 30 '24 14:07 jits

The withProps feature has been added in v19.

I'm going to close this issue.

markostanimirovic avatar Dec 11 '24 11:12 markostanimirovic