Skip to content
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

feat(entity): introduce upsert methods #780

Merged
merged 2 commits into from
Feb 6, 2018
Merged

Conversation

sandangel
Copy link
Contributor

This PR continue some works of @dinvlad , include some fix:

  • change function return type from boolean to DidMutate
  • return correct DidMutate based on update or add

Refs: #550
Closes: #421

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 93.023% when pulling cd8c211 on sandangel:master into a140fa9 on ngrx:master.

@dinvlad
Copy link
Contributor

dinvlad commented Feb 6, 2018

Thanks @sandangel for picking up the baton - apologies again for not finding time for it. Your PR looks great!

@MikeRyanDev MikeRyanDev merged commit f871540 into ngrx:master Feb 6, 2018
@shahafal
Copy link

shahafal commented Feb 6, 2018

@sandangel
hi i'll write here what i wrote in dinvlad pr, as the pr was closed before anyone could address it.

as opposed to the simple update case, where perhaps we pass an object id and a change and can assume there is a change,
in upsert we'll porobably pass a list of full-blown objects to either be added if missing or updated if exists.
in such case, we might add objects that exists and do not need any update because they are exactly the same as they were before.

i went over your code, and just like in dinvlad's pr -
in the current implementation what would happen is that the objects will be updated, thus assigned a new reference. this would cause them, given angular's change detection on-push strategy, to reload into the page.

do you think this can be addressed?

@sandangel
Copy link
Contributor Author

hi @shahafal
What you want to address does not relate to my PR, it relates to update methods. upsert methods is just detect whether id in entities and call addMany, updateMany accordingly.

@shahafal
Copy link

shahafal commented Feb 6, 2018

@sandangel
you are right that this is a performance issue that actually effects the update method,
however,
when you do a regular update you specify a change you want to make, which means you are more likely to pass a real change and not the same data.

and this brings me to why i think it is related to your pr too -
due to the fact that you'd expect upsert to get list of full blown entities that might not necessarily contain changes in them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entity: adapter.addOrUpdate
5 participants