@ngrx/signals: updateEntity method don't update key in entityMap

[xSirrioNx]

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

signals

Minimal reproduction of the bug/regression with instructions

interface Todo {
  id: string;
  name: string;
}

const TodoStore = signalStore(withEntities<Todo>());

// Add model with temp id to store
patchState(TodoStore, addEntity({id: 'tmp_id', name: 'some_name'}))

/**
 * At whis point we have TodoStore.entities() = [{id: 'tmp_id', name: 'some_name'}]
 * and TodoStore.entityMap() = { 'tmp_id':  {id: 'tmp_id', name: 'some_name'}}
 */

// Then i wants to update entity with real data, for example from API
patchState(TodoStore, updateEntity({id: 'tmp_id', changes: {id: 'real_uuid_from_api', name: 'some_real_name'}}))

/**
 * Now we have TodoStore.entities() = [{id: 'real_uuid_from_api', name: 'some_real_name'}]
 * ❗and TodoStore.entityMap() = { 'tmp_id':  {id: 'real_uuid_from_api', name: 'some_real_name'}}❗
 */

// Then i want's to delete this entity, but nothing happens and state of entities don't changed
patchState(TodoStore, removeEntity('real_uuid_from_api'))

/**
 * I think, this because of removeEntity try to find id in entityMap
 * but entityMap was not changed bu updateEntity method
 */

Expected behavior

If i call updateEntity method with changed entity ID, entityMap should be updated too

Against 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

• [ ] Yes
• [X] No

[SerkanSipahi]

@markostanimirovic

I can confirm this behaviour. We have the same expectation as @xSirrioNx . Additionally to store.entityMap, store.ids() contains also the old tmp id instead of the new id.

[ducin]

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 id. Either don't put the object into the store (if the backend hasn't assigned an id, it's NOT an entity YET) - or make the frontend assign a GUID and send it to the backend. I'd go with putting the entity into the store after it's returned from the server (with ID)

to make types bullet proof, I'd go with "strict unions" - disable the ability to pass the id at all:

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

[markostanimirovic]

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, @ngrx/entity provides the ability to change the ID, so this change may land for compatibility.

[SerkanSipahi]

so this change may land for compatibility

Thank you @markostanimirovic

@markostanimirovic @ducin

IMO changing an ID is doable but is a very bad idea and should not be done

I'm also not a big fan of updating the ID.

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 ✌️☮️

[xSirrioNx]

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

[markostanimirovic]

This will be a breaking change for entities whose identifier name is different from id. In other words, it will be required to pass idKey in case of a custom identifier for all update... entity updaters which is not the case with the current implementation.

Even though the @ngrx/signals package is in a developer preview, we try to minimize breaking changes, so there is a chance that this change will land in v18 instead of v17.x.

[SerkanSipahi]

@markostanimirovic is there any workaround in the meantime?

[markostanimirovic]

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

[SerkanSipahi]

@markostanimirovic thanks.

[SerkanSipahi]

@markostanimirovic will this land in v18?

[markostanimirovic]

@markostanimirovic will this land in v18?

Yes.

[SerkanSipahi]

@markostanimirovic thank you for https://github.com/ngrx/platform/pull/4404