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] Potential typing bug when using discriminated unions #4467

Open
1 of 2 tasks
jits opened this issue Jul 30, 2024 · 9 comments
Open
1 of 2 tasks

[signals] Potential typing bug when using discriminated unions #4467

jits opened this issue Jul 30, 2024 · 9 comments

Comments

@jits
Copy link
Contributor

jits commented Jul 30, 2024

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

  • Yes
  • No
@rainerhahnekamp
Copy link
Contributor

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.

@jits
Copy link
Contributor Author

jits commented Jul 30, 2024

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

@rainerhahnekamp
Copy link
Contributor

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

@jits
Copy link
Contributor Author

jits commented Jul 30, 2024

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

@markostanimirovic
Copy link
Member

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.

@jits
Copy link
Contributor Author

jits commented Jul 30, 2024

@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):

@rainerhahnekamp
Copy link
Contributor

@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 });
      },
    };
  })
);

@markostanimirovic
Copy link
Member

@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

@jits
Copy link
Contributor Author

jits commented Jul 30, 2024

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

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