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

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

Closed
2 tasks
Raul52 opened this issue Jan 29, 2024 · 4 comments · Fixed by #4396
Closed
2 tasks

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

Raul52 opened this issue Jan 29, 2024 · 4 comments · Fixed by #4396
Labels
Blocked (External) Blocked by external package Project: Signals

Comments

@Raul52
Copy link

Raul52 commented Jan 29, 2024

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:

  • usingType() function is used to mimick what any set* method would do if you accessed the config object.

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

  • Yes
  • No
@markostanimirovic
Copy link
Member

@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
Copy link
Author

Raul52 commented Jan 29, 2024

@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
Copy link
Member

I opened an issue in the TS repo: microsoft/TypeScript#57318

As a workaround, you can use conditional type: example

@markostanimirovic markostanimirovic added Blocked (External) Blocked by external package and removed bug labels Feb 7, 2024
@markostanimirovic
Copy link
Member

gisbdzhch pushed a commit to gisktzh/gb3-web_ui that referenced this issue Aug 13, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@ngrx/schematics](https://github.com/ngrx/platform) | devDependencies | major | [`^17.1.1` -> `^18.0.0`](https://renovatebot.com/diffs/npm/@ngrx%2fschematics/17.2.0/18.0.1) |

---

### Release Notes

<details>
<summary>ngrx/platform (@&#8203;ngrx/schematics)</summary>

### [`v18.0.1`](https://github.com/ngrx/platform/blob/HEAD/CHANGELOG.md#1801-2024-06-27)

[Compare Source](ngrx/platform@18.0.0...18.0.1)

##### Bug Fixes

-   **component-store:** resolve issues in migration script to v18 ([#&#8203;4403](ngrx/platform#4403)) ([a15d53e](ngrx/platform@a15d53e))
-   **effects:** fix bugs in migration script to v18 ([#&#8203;4402](ngrx/platform#4402)) ([6ae4723](ngrx/platform@6ae4723))
-   **eslint-plugin:** only take return statement into account with no-multiple-actions-in-effects ([#&#8203;4410](ngrx/platform#4410)) ([c9c646c](ngrx/platform@c9c646c)), closes [#&#8203;4409](ngrx/platform#4409)

##### Features

-   **signals:** rename signals to computed when defining custom features with input ([#&#8203;4395](ngrx/platform#4395)) ([05f0940](ngrx/platform@05f0940)), closes [#&#8203;4391](ngrx/platform#4391)
-   **signals:** replace `idKey` with `selectId` when defining custom entity ID ([#&#8203;4396](ngrx/platform#4396)) ([67a5a93](ngrx/platform@67a5a93)), closes [#&#8203;4217](ngrx/platform#4217) [#&#8203;4392](ngrx/platform#4392)

### [`v18.0.0`](https://github.com/ngrx/platform/blob/HEAD/CHANGELOG.md#1800-2024-06-13)

[Compare Source](ngrx/platform@17.2.0...18.0.0)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or rename PR to start with "rebase!".

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked (External) Blocked by external package Project: Signals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants