ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
7.96k stars 1.95k forks source link

Type of idKey is not recognized correctly when using generics #4217

Closed Raul52 closed 3 weeks ago

Raul52 commented 5 months ago

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

signals

Minimal reproduction of the bug/regression with instructions

I am trying to create a signal store feature using generics and I stumbled upon a typing error when trying to use the idKey: https://ngrx.io/guide/signals/signal-store/entity-management#customized-id-property

I have the following feature:

export interface Entity {
    uuid: EntityId;
}

export function withEntityLoader<E extends Entity, F, S extends LoaderService<E, F>>(loaderServiceType: Type<S>, filter: F) {
    return signalStoreFeature(
    ......
        withMethods((store) => {
            const dataService = inject(loaderServiceType);

            return {
                load() {
                    rxMethod<void>(
                    ....................................................
                                    tapResponse({
                                        next: response => {
                                            patchState(store, setAllEntities(response.items, {idKey: 'uuid'}));
                                        },
                                    }),
                                ),
                            ),
                        ),
                    );
                },
            };
        }),
    );
}

I checked how the type definition is made internally and tried to reproduce it with as concise an example as I could.

context:

Current faulty behavior

Expected behavior

The expected behavior is that uuid is recognized as a key with the correct type.

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

"@ngrx/signals": "17.1.0"

Other information

If you change the type of EntityIdProps to

export type EntityIdProps<Entity> = {
    [K in keyof Entity as K extends EntityId ? K : never]: Entity[K];
};

Working playground

Then you will have the correct typing.

Seems like the type is not correctly resolved when using the indexed property accessor via generics which extends a known type.

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

markostanimirovic commented 5 months ago

@Raul52

export type EntityIdProps<Entity> = {
    [K in keyof Entity as K extends EntityId ? K : never]: Entity[K];
};

👆 By applying the changes that you proposed, the EntityIdProps type will always return all props because K extends EntityId will always be true (except when some key is a symbol).


The idea with:

export type EntityIdProps<Entity> = {
    [K in keyof Entity as Entity[K] extends EntityId ? K : never]: Entity[K];
};

is to exclude all properties whose values are not string | number (not keys), because they cannot be used as identifiers.


But anyway, thanks for reporting this! Let's try to find some workaround.

Raul52 commented 5 months ago

@markostanimirovic I created a complete test case and you are right, my proposed solution was wrong.

test-case

Will also actively check for a solution given the intention - which is to allow keys whose values are `string | number', but not other types.

markostanimirovic commented 5 months ago

I opened an issue in the TS repo: https://github.com/microsoft/TypeScript/issues/57318

As a workaround, you can use conditional type: example

markostanimirovic commented 5 months ago

https://github.com/microsoft/TypeScript/issues/48992