feat(Entity): add updateMany by predicate #907

timdeschryver · 2018-03-13T18:42:21Z

This PR allows to edit entities based on a predicate.
See #672, in the issue it was mentioned to update the entities via multiple predicates - see my comment as why I don't think thats a good idea. If needed I can implement it that way tho.

I do got one question about the implementation of the sorted state adapter.
Why does this have an own implementation?
In my head the sorted adapter should call the unsorted adapter and do the sorting afterwards, but I must be missing someting 😅

coveralls · 2018-03-13T18:44:47Z

Coverage increased (+0.07%) to 88.437% when pulling 3680a79 on timdeschryver:pr/update-many-predicate into a9bc070 on ngrx:master.

MikeRyanDev · 2018-03-15T00:48:45Z

Implementation seems reasonable. What are your opinions on this:

Instead of a predicate with a list of changes the signature is updated to use a filter/map function:

 // Name is bad but it illustrated the interface
interface Updater<T> {
  (entity: T): false | Change<T>;
}
function updateMany(updater: Updater<T>, state: EntityState<T>);

timdeschryver · 2018-03-15T12:07:48Z

I don't think I get the idea.
Would this be a map implementation where the user implements map?
For example:

adapter.updateMany(
      entity => entity.id.startsWith('a') ? {title: 'New title'} : false, // maybe null or an empty object instead of false?
      withMany
);

MikeRyanDev · 2018-03-15T12:24:03Z

Yes, that's right.

timdeschryver · 2018-03-15T18:11:29Z

I must say your proposal feels a bit weird at first but after seeing it in action, it is better in my opinion.
I did change the signature to return a Partial<T>, this brings the flexibility to do a second change - if no change is required we can return an empty object or the entitity object itself.
Thoughts?
See the unsorted state test which illustrates an additional change: book => (book.id.startsWith('a') ? firstChange : secondChange),

If someone got a better name than Change please feel free to speak up 😄

EDIT: I just realized that with your signature, it is also possible to have a second change.
Matter of fact, it is possible to introduce as many changes as needed.
I'll change the code so it has a false | Partial<T> signature.

MikeRyanDev · 2018-03-19T02:40:16Z

modules/entity/src/models.ts

@@ -38,6 +38,10 @@ export type UpdateNum<T> = {

 export type Update<T> = UpdateStr<T> | UpdateNum<T>;

+export type Map<T> = {


Recommend renaming this to something more specific. Maybe UpdateMap?

MikeRyanDev · 2018-03-19T02:42:47Z

modules/entity/src/sorted_state_adapter.ts

+ const updates: Update<T>[] =
+ updatesOrMap instanceof Array
+ ? updatesOrMap
+ : state.ids.map((id: string | number) => ({


I think that creating an Update object for each entity means even entities that shouldn't change will lose referential equality. I think this map should be a reduce instead that shrinks the ids array into a smaller array of Updates for only changed entities.

MikeRyanDev · 2018-03-19T17:14:25Z

modules/entity/src/sorted_state_adapter.ts

+ : state.ids.reduce((changes: Update<T>[], id: string | number) => {
+ const change = updatesOrMap(state.entities[id]);
+ if (change && Object.keys(change).length > 0) {
+ return [


Recommend changing this line to:

changes.push({ id, changes: change }); return changes;

That way we don't allocate and deallocate an array for every item we are upserting.

Done, thanks for the feedback/reviews!

@tdeschryver Thanks for all of the hard work! We really appreciate it! 😄

MikeRyanDev · 2018-03-19T17:14:45Z

One minor change. Otherwise this is looking good to me!

sandangel · 2018-04-11T21:54:20Z

Hi, how about adding this feature to upsert too? When predicate function is provided, upsert will call update with predicate under the hood

brandonroberts · 2018-09-14T02:48:30Z

@timdeschryver will you rebase this so it can land?

timdeschryver · 2018-09-14T06:10:52Z

✔️

MikeRyanDev · 2018-09-14T06:16:28Z

I wonder if we could just have entityAdapter.map(...) instead.

timdeschryver · 2018-09-14T06:26:19Z

That would make sense, since this is what we're actually doing.
But I think having a updateMany would have its value if we would change the API to the following ?

{
  predicate: entity => boolean,
  changes: Partial<Entity>
}

timdeschryver · 2018-09-14T06:27:29Z

Ha, now I see we started with the above API 😄 - 149042b

timdeschryver · 2018-09-22T14:31:49Z

I've updated the PR to be a map function.
While doing this I also changed the signature to actually return an entity object.

Let me know your thoughts about this one.
I could also submit a PR if you agree with #907 (comment).

brandonroberts · 2018-10-25T13:45:13Z

modules/entity/src/models.ts

@@ -38,6 +38,10 @@ export type UpdateNum<T> = {

 export type Update<T> = UpdateStr<T> | UpdateNum<T>;

+export type EntityMap<T> = {


Update type to match export type ComparerStr<T> = (a: T, b: T) => string;

brandonroberts · 2018-10-25T13:46:00Z

Minor nit, otherwise changes LGTM

timdeschryver · 2018-10-25T17:19:38Z

There we go 😄

brandonroberts · 2018-10-26T13:36:08Z

@timdeschryver rebase pls

timdeschryver · 2018-10-26T17:43:19Z

modules/entity/src/index.ts

+ EntityState,
+ EntityAdapter,
+ Update,
+ EntityMap,


I did also add EntityMap and Predicate here.

brandonroberts · 2018-10-26T18:09:29Z

modules/store/src/store.ts

@@ -19,9 +19,6 @@ export class Store<T> extends Observable<T> implements Observer<Action> {
 this.source = state$;
 }

- /**


Revert this change because its in a separate PR

Doh, I was using 2 VSCode instances and was wondering what was happening.
Sorry... 😅

timdeschryver mentioned this pull request Mar 14, 2018

feat(Entity): add removeMany by predicate #900

Merged

MikeRyanDev reviewed Mar 19, 2018

View reviewed changes

MikeRyanDev added Project: Entity 7.x labels Mar 19, 2018

MikeRyanDev reviewed Mar 19, 2018

View reviewed changes

MikeRyanDev added the Ready for Merge [7.x] label Mar 19, 2018

timdeschryver mentioned this pull request Mar 19, 2018

RFC: Entity: provide changes with map function #671

Closed

timdeschryver force-pushed the pr/update-many-predicate branch from cc1b99b to def18cb Compare September 14, 2018 06:09

timdeschryver force-pushed the pr/update-many-predicate branch from def18cb to cf9023d Compare September 22, 2018 13:54

brandonroberts requested changes Oct 25, 2018

View reviewed changes

timdeschryver force-pushed the pr/update-many-predicate branch from 68c47a7 to 03a85fa Compare October 25, 2018 17:09

timdeschryver added 2 commits October 26, 2018 19:28

feat(Entity): add updateMany by predicate

00f329a

fix typo

e175945

timdeschryver and others added 9 commits October 26, 2018 19:30

change signature

f80d51b

rename to change

64bd794

inline decideIdsToUpdate

7572137

rename to map and allow false as a map option

47f0452

reduce changes

fa7179f

extra check on empty objects

2bf103b

push change to changes

967bd65

feat(Entity): add map function

8cca796

add new types to export

3c24b3d

timdeschryver force-pushed the pr/update-many-predicate branch from 03a85fa to 3c24b3d Compare October 26, 2018 17:40

timdeschryver commented Oct 26, 2018

View reviewed changes

brandonroberts reviewed Oct 26, 2018

View reviewed changes

timdeschryver force-pushed the pr/update-many-predicate branch from 3680a79 to 3c24b3d Compare October 26, 2018 18:12

brandonroberts approved these changes Oct 26, 2018

View reviewed changes

brandonroberts merged commit 4e4c50f into ngrx:master Oct 26, 2018

timdeschryver deleted the pr/update-many-predicate branch October 26, 2018 18:20

timdeschryver mentioned this pull request Oct 29, 2018

RFC: Entity: update\remove by predicate #672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Entity): add updateMany by predicate #907

feat(Entity): add updateMany by predicate #907

timdeschryver commented Mar 13, 2018 •

edited

Loading

coveralls commented Mar 13, 2018 •

edited

Loading

MikeRyanDev commented Mar 15, 2018

timdeschryver commented Mar 15, 2018

MikeRyanDev commented Mar 15, 2018

timdeschryver commented Mar 15, 2018 •

edited

Loading

MikeRyanDev Mar 19, 2018

timdeschryver Mar 19, 2018

MikeRyanDev Mar 19, 2018

MikeRyanDev Mar 19, 2018

timdeschryver Mar 19, 2018

MikeRyanDev Mar 19, 2018

MikeRyanDev commented Mar 19, 2018

sandangel commented Apr 11, 2018 •

edited

Loading

brandonroberts commented Sep 14, 2018

timdeschryver commented Sep 14, 2018

MikeRyanDev commented Sep 14, 2018

timdeschryver commented Sep 14, 2018

timdeschryver commented Sep 14, 2018 •

edited

Loading

timdeschryver commented Sep 22, 2018 •

edited

Loading

brandonroberts Oct 25, 2018

brandonroberts commented Oct 25, 2018

timdeschryver commented Oct 25, 2018

brandonroberts commented Oct 26, 2018

timdeschryver Oct 26, 2018 •

edited

Loading

brandonroberts Oct 26, 2018

timdeschryver Oct 26, 2018

		@@ -38,6 +38,10 @@ export type UpdateNum<T> = {

		export type Update<T> = UpdateStr<T> \| UpdateNum<T>;

		export type Map<T> = {

		@@ -38,6 +38,10 @@ export type UpdateNum<T> = {

		export type Update<T> = UpdateStr<T> \| UpdateNum<T>;

		export type EntityMap<T> = {

feat(Entity): add updateMany by predicate #907

feat(Entity): add updateMany by predicate #907

Conversation

timdeschryver commented Mar 13, 2018 • edited Loading

coveralls commented Mar 13, 2018 • edited Loading

MikeRyanDev commented Mar 15, 2018

timdeschryver commented Mar 15, 2018

MikeRyanDev commented Mar 15, 2018

timdeschryver commented Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeRyanDev commented Mar 19, 2018

sandangel commented Apr 11, 2018 • edited Loading

brandonroberts commented Sep 14, 2018

timdeschryver commented Sep 14, 2018

MikeRyanDev commented Sep 14, 2018

timdeschryver commented Sep 14, 2018

timdeschryver commented Sep 14, 2018 • edited Loading

timdeschryver commented Sep 22, 2018 • edited Loading

Choose a reason for hiding this comment

brandonroberts commented Oct 25, 2018

timdeschryver commented Oct 25, 2018

brandonroberts commented Oct 26, 2018

timdeschryver Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdeschryver commented Mar 13, 2018 •

edited

Loading

coveralls commented Mar 13, 2018 •

edited

Loading

timdeschryver commented Mar 15, 2018 •

edited

Loading

sandangel commented Apr 11, 2018 •

edited

Loading

timdeschryver commented Sep 14, 2018 •

edited

Loading

timdeschryver commented Sep 22, 2018 •

edited

Loading

timdeschryver Oct 26, 2018 •

edited

Loading