Akita: Allow update/remove entities by a predicate (not just IDs)

[DmitryEfimenko]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[X] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

It'd be great if we could update and remove items from the store based on anything else but IDs.
The overloads that would be added would look something like:

update(predicate: ((entity: Readonly<E>) => boolean), newStateFn: ((entity: Readonly<E>) => Partial<E>));
update(predicate: ((entity: Readonly<E>) => boolean), newStateFn: Partial<E>);
update(predicate: ((entity: Readonly<E>) => boolean), newState: Partial<S>);

this would allow to write this:

renameAllNicks() {
  this.update(e => e.name === 'Nick', { name: 'Bob' });
}

I understand that the same result could be achieved with this:

this.update(null, e => e.name === 'Nick' ? { ...e, name: 'Bob' } : e);

...but, that looks more verbose (and this is a very simple example).

Things are a bit worse for the remove case. Instead of proposed suggestion:

this.remove(e => e.name === 'Nick');

...today's solution would have to be the following:

const idsToRemove = [];
for (const id in this.entities) {
  if (this.entities[id].name === 'Nick') {
    idsToRemove.push(id);
  }
}
this.remove(idsToRemove);

I could take a stab at this and submit a PR if you give me a "good to go".

[NetanelBasal]

Sounds good. You are good to go. Just tell me what is the Api exactly.

[DmitryEfimenko]

Changes are to be performed for EntityStore's API.
The following three overloads would be added for update method:

update(predicate: ((entity: Readonly<E>) => boolean), newStateFn: ((entity: Readonly<E>) => Partial<E>));
update(predicate: ((entity: Readonly<E>) => boolean), newStateFn: Partial<E>);
update(predicate: ((entity: Readonly<E>) => boolean), newState: Partial<S>);

The following overload would be added to remove method:

remove(predicate: ((entity: Readonly<E>) => boolean));

[DmitryEfimenko]

while looking at the code, I may have spotted a bug:
There is an overload on EntityStore that's not documented, but it shows in intellisense:

update(newState: Partial<S>);

this overload is not handled.
I tried writing a quick unit test for it and it failed:

it('should update all - 2', () => {
  store.add(new Todo({ id: 1 }));
  store.add(new Todo({ id: 2 }));
  store.update({ title: 'update' });
  expect(store.entities[1].title).toEqual('update');
  expect(store.entities[2].title).toEqual('update');
});

Is it ok if I remove this overload as a part of my code change so that the users aren't confuced?

Also, while asking questions. How do I make an update to the documentation? Or will you be doing that?

[NetanelBasal]

I will do the update.
The overload is a typescript thing, if you remove it, it will yell. ( if you have a way to solve it, go for it)

Ok, the API looks good, let's go :)

[DmitryEfimenko]

yeah, I see what you mean about "typescript thing". It yells and I can't figure out quickly how to fix it. A shame...

[DmitryEfimenko]

while poking around, I may have found a bug after all :)
The following test will fail:

it('should update many with callback using entity object', () => {
  store.add(new Todo({ id: 1 }));
  store.add(new Todo({ id: 2 }));
  store.add(new Todo({ id: 3 }));
  store.update([1, 2], entity => ({ title: 'update' + entity.id }));
  expect(store.entities[1].title).toEqual('update1');
  expect(store.entities[2].title).toEqual('update2');
  expect(store.entities[3].title).toEqual('3');
});

This is due to this line. It's not accounting for the fact that stateOrId may be an ID[] or null.

I'll address this since I'm updating this function if you don't mind