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

RFC: Entity: update\remove by predicate #672

Closed
aigosh opened this issue Dec 27, 2017 · 6 comments
Closed

RFC: Entity: update\remove by predicate #672

aigosh opened this issue Dec 27, 2017 · 6 comments

Comments

@aigosh
Copy link

aigosh commented Dec 27, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x] Feature request
[ ] Documentation issue or request

What is the current behavior?

Current behaviour does not let us update or remove many entities by predicate.

Expected behavior:

There is often a need to update or remove by the way described.
So it would be great to have an ability to update\remove many entities this way:

adapter.updateMany([
        {
            predicate: entity => entity.someField === someValue,
            changes: {
                someAnotherField: someAnotherValue
            }
        }])

adapter.removeMany(entity => entity.someField === someValue, predicate1, predicate2, ...)

Also some there are some cases that requires attention:

  • there could be several conditions, depending on which could be required different changes
  • there should be no breaking changes

Other information:

Please look at the issue #671. It is tightly tied to this one, because it would be great to provide predicate and changes function at the same time.

@brandonroberts
Copy link
Member

If you want to filter based on some predicate, why not find the ids based on the predicate and use them to remove items?

@aigosh
Copy link
Author

aigosh commented Jan 16, 2018

Yes, I can do as you say, but I find this useful, to have this feature "out of the box".

@timdeschryver
Copy link
Member

I'll take this one 😄

@timdeschryver
Copy link
Member

timdeschryver commented Mar 9, 2018

While implementing the updateMany, I came up with an edge case and would like some feedback.

Would it be clear if we let the option open to add multiple predicates at once?
If so, what is the desired behaviour when a model satisfies multiple predicates?

If it should be possible I would opt to accumulate all changes, but maybe this would raise some questions with users.
If it shouldn't be possible we don't have this 'problem'. This has my favor because then the intention is very clear, and I also can't think of a use case where I want to be able to update entities by using multiple predicates

For example:

Given state: [
  {id: 1, title 'Title 1', description: 'Description 1'}, 
  {id: 2, title 'Title 2', description: 'Description 2'}
]

adapter.updateMany([
  { predicate: book => book.id === 1, changes: { title : 'Changed title 1', description: 'Awesome description' }},
  { predicate: book => book.id < 5, changes: { title : 'Awesome title' }}
])

Output: [
  {id: 1, title 'Awesome title', description: 'Awesome description'}, 
  {id: 2, title 'Awesome title', description: 'Description 2'}
]

@cburnicki
Copy link

Would be great to have this feature. However, I would argue that in favor of simple, atomic operations, you should only be able to pass one predicate per operation. The first signature that came to my mind would look like this:
export interface Predicate<T> { (entity: T): boolean }

updateMany<S extends EntityState<T>>(updates: Update<T>[], state: S, predicate: Predicate<T>): S;

So the changes would only be applied to entities that match a certain predicate which should be bound to the operation you are performing, not the change itself.

To add another usecase (optimistic UI, image upload):

  1. User creates new post with image
  2. New post is added to server and in the app state with some kind of isLoading: true attribute.
  3. Now that the post itself has been added in the backend (and has an id), the image upload starts and might fail.
  4. In the meantime, the app issues a GET request to receive existing posts from the server.
  5. Now our newly created post has no image on the server (since the upload hasn't happened), but it does have in image in the app state.

It would be great to be able to add a post => post.isLoading === false predicate to the upsertMany statement to make sure our new post will not be overwritten and lose the image.

@timdeschryver
Copy link
Member

Closing this because #907 and #900 landed.

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

No branches or pull requests

5 participants