fix(signals): allow modifying entity id on update

markostanimirovic commented 3 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

[x] The commit message follows our guidelines: https://github.com/ngrx/platform/blob/main/CONTRIBUTING.md#commit
[x] Tests for the changes have been added (for bug fixes / features)
[x] Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

It's not possible to modify entity ID via update* functions. Instead, it's necessary to remove the existing one and add a new entity with an updated ID:

const myEntity = { ...entityMap()['tmp_id'], id: 'real_id' };

patchState(store, removeEntity('tmp_id'), addEntity(myEntity));

Closes #4235

What is the new behavior?

The update* updaters can be used to modify entity ID:

patchState(store, updateEntity({ id: 'tmp_id', changes: { id: 'real_id' } }));

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

After this change, it's necessary to provide selectId to all update* updaters if an entity has a custom ID.

type Todo = { _id: string; text: string };

+const selectId: SelectEntityId<Todo> = (todo) => todo._id;

const TodosStore = signalStore(
  withEntities<Todo>(),
  withMethods((store) => ({
    updateTodo(id: string, changes: Partial<Todo>): void {
-      patchState(store, updateEntity({ id, changes }));
+      patchState(store, updateEntity({ id, changes }, { selectId }));
    },
  }))
);

This is not considered as a breaking change because the @ngrx/signals package is in developer preview.

netlify[bot] commented 3 months ago

Deploy Preview for ngrx-io canceled.

Name	Link
Latest commit	9990abd78e0859473c17bae23dfe3e5d8725d53e
Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/668043d1d1f6b00009655a6d

markostanimirovic commented 3 months ago

@timdeschryver With allowing ID changes there are several ways to break/move entity collection to the inconsistent state. That's the reason why I was against this change initially. Users now have better control, but also responsibility.

Let's check how @ngrx/entity handles this case. It probably works the same.

I agree that warning will be useful 👍

markostanimirovic commented 3 months ago

I just checked @ngrx/entity - the behavior is the same. Let's add a warning then.

ngrx / platform