-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
@ngrx/signals: updateEntity method don't update key in entityMap #4235
Comments
I can confirm this behaviour. We have the same expectation as @xSirrioNx . |
IMO changing an ID is doable but is a very bad idea and should not be done, as it's a design mistake to change IDs ever. IDs are meant to be immutable (never change), as they identify entities across systems. Guessing from your code, there is some time span where you don't have any to make types bullet proof, I'd go with "strict unions" - disable the ability to pass the declare function disable_id_from_partial<T>(withoutId: Partial<T> & { id?: undefined }): void;
disable_id_from_partial<Todo>({ name: 'some_real_name' }) // ✅ OK
disable_id_from_partial<Todo>({ id: 'uuid', name: 'some_real_name' }) // ❌ ID - NOT OT |
I'm also not a big fan of updating the ID. 👀 That's the reason why the current implementation does not support it. On the other hand, |
Thank you @markostanimirovic
I understand your concerns, but there are legitimate cases where it may be necessary to create a temporary ID. If you guys are interested, I can go into this topic in more detail. However, I think the decision of what do to with the id belongs to the developer or team ✌️☮️ |
So we can add separeted method to change entity ID For example: patchState(store,
updateEntity({ id: 'old_id', changes: {id: 'new_id'}}),
changeEntityId('old_id', 'new_id')
) |
This will be a breaking change for entities whose identifier name is different from Even though the |
@markostanimirovic is there any workaround in the meantime? |
You can do: const myEntity = { ...entityMap()['tmp_id'], id: 'real_id' };
patchState(store, removeEntity('tmp_id'), addEntity(myEntity)); |
@markostanimirovic thanks. |
@markostanimirovic will this land in v18? |
Yes. |
@markostanimirovic thank you for #4404 |
Which @ngrx/* package(s) are the source of the bug?
signals
Minimal reproduction of the bug/regression with instructions
Expected behavior
If i call
updateEntity
method with changed entity ID,entityMap
should be updated tooAgainst we can't remove model by new setted ID
Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)
"@ngrx/signals": "^17.1.0",
"@angular/core": "~17.0.8",
Node: v21.4.0
OS: Win 11
Browser: MS Edge
Other information
No response
I would be willing to submit a PR to fix this issue
The text was updated successfully, but these errors were encountered: