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

Add a method for adding multiple items to the collection at once #7627

Closed
mlewand opened this issue Jul 15, 2020 · 10 comments · Fixed by #7644
Closed

Add a method for adding multiple items to the collection at once #7627

mlewand opened this issue Jul 15, 2020 · 10 comments · Fixed by #7644
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:utils type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@mlewand
Copy link
Contributor

mlewand commented Jul 15, 2020

📝 Provide a description of the new feature

When working on #6194 we figured that it would be good to have a way to add multiple items into a collection at once.

The tl;dr motivation is that when the colleciton changes, a resource-heavy logic needs to be run. Currently the only option is to listen to add event and run the logic for each added item - which is inefficient.

Instead a addBatch method and event pair will solve this.

Note that this new method needs to play well with existing add event. Also calling collection.add() should trigger batchAdd event too.


If you'd like to see this feature implemented, add a 👍 reaction to this post.

@mlewand mlewand added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). intro Good first ticket. package:utils domain:dx This issue reports a developer experience problem or possible improvement. squad:red labels Jul 15, 2020
@mlewand mlewand added this to the iteration 34 milestone Jul 15, 2020
@mlewand mlewand self-assigned this Jul 15, 2020
@mlewand
Copy link
Contributor Author

mlewand commented Jul 15, 2020

I'm wondering whether we should add also a complimentary removeBatch funciton in one go.

While it is possible to keep similar interface similarity between collection.add() and proposed batchAdd:

The case with removeBatch is a little more tricky, because remove accepts subject (reference of the item to be removed).

Few options that I see here are:

  • Skip the removeBatch for now (we don't need it anywhere for the time being).
  • removeBatch( first:Object[, last:Object] )
  • removeBatch( first:Number[, last:Number] )
  • removeBatch( offset:Number[, howMuch:Number] )
  • removeBatch( first:Object[, howMuch:Number] )

@jodator
Copy link
Contributor

jodator commented Jul 15, 2020

My 2¢:

  • the addBatch() is confusing - I was thinking about adding a model's Batch to something.
  • look how Backbone solves this: https://backbonejs.org/#Collection-add there's an add event for every object and update event after all objects added.

The name is terrible for me. If not extending add() as in backbone, maybe addAll() would be better?

@oleq
Copy link
Member

oleq commented Jul 15, 2020

Yeah, I also like the way backbone handles this. But they have the first argument an array (iterable) by default and we cannot do this because this would mean a BC in 1000 different places. We have to support:

  • add( singleItem )
  • add( singleItem, position )

and this collides with the add( iterableOfMultipleItems ) approach because we also support adding arrays (maps, sets...) to Collections. We wouldn't know whether its a singleItem or iterableOfMultipleItems.

OTTH we cannot do add( firstItem, secondItem ) either because in some cases secondItem would be treated as position (add( singleItem, position )).

@mlewand
Copy link
Contributor Author

mlewand commented Jul 15, 2020

We can't extend add at this point without a BC.

  • add( item, item1, itemN, index ) will not work, because you can't tell if 2 in add( 1, 2 ) means an index or a value to be inserted
  • add( arrayOfItems[, index] ) will requre a BC that the Collection does not support array values (currently it's possible to do add( [ 1, 2 ] ) add( [ 'foo', 'bar' ]

The "batch" word is meant to underline that's an insertion of a bigger sequence into an already existing data strucutre. IMHO getAll() doesn't tell the method purpose, while addBatch, addMultiple or addMany say more why this function exists next to a regular add.

Not sure about the update event, if thinking like that changed event sounds more relevant. But such event would make sense in the collection all in all IMHO.

@oleq
Copy link
Member

oleq commented Jul 15, 2020

So, long story short, we need another method (which I personally hate). I also don't like this model batch connotation. Maybe addMultiple() or addMany() instead? Some ideas:

@mlewand
Copy link
Contributor Author

mlewand commented Jul 15, 2020

And JavaScript has concat and splice 😄

  • concat - its syntax and purpose is in line with the subject of this issue, BUT its immutable (returns a new array, rather than modify state of this array) - this could be a trap for people who blindly assume identical behavior
  • splice - well it does what it should, but imho it's one of these screwed up API methods that does too many things (remove and push at the same time)

That being said, I can see the reason why we should avoid word "batch" in v5, so addMany or extend sounds also ok.

However in case of changing the name also the event name should changed. I could either go with addMany or implement more agnostic change event - but if we go with this it should already account for removing too. So the following API makes sense for me:

this.fire( 'change', {
    added: [ newItem1, newItem2 ],
    addedIndex: 0,

    removed: [ removedItem1, removedItem2 ],
    removedIndex: 4
} );

@jodator
Copy link
Contributor

jodator commented Jul 16, 2020

  • addMany / addAll 👍
  • change 👍
  • extended event data - 👍 - AFIR backbone creates a similar structure for "update" event

The adde/removedIndex might be just index I don't see ATM splice implementation for collections.

@mlewand
Copy link
Contributor Author

mlewand commented Jul 16, 2020

If we were to simplify the change event structure to support only adding or removing (but not both) then we'll have to fire two events in case of change that removes and adds stuff at once. The cases that I'm aware of:

  • set - if we implement a method that allows to override collection's items - sounds pretty likely
  • splice equivalent - I guess we can agree that's unlikely happen

Also in this case the event should be further simplified, instead added and removed there should be one prop, like items so that it is easier to memorize.

{
	type: 'add', // or 'remove'
	items: Iterable.< x, y, z >,
	index: 0
}

Again, going this direction will mean that if we implement Collection.set() at some point it will fire two events change( { type: 'remove' } ) and change( { type: 'add' } ).

I'd opt for going #7627 (comment) so that we won't have this problem in the future. In the end, typically you just listen for change event without actually checking what really happened.

@jodator
Copy link
Contributor

jodator commented Jul 16, 2020

If we were to simplify the change event structure to support only adding or removing (but not both) then we'll have to fire two events in case of change that removes and adds stuff at once. The cases that I'm aware of:

Hmmm, what about not worrying about that - again looking at the Backbone data it could be:

 fire( 'change', {
 	added: [],
 	removed: [],
 	index: 0
 } );

@mlewand
Copy link
Contributor Author

mlewand commented Jul 16, 2020

Might work, using a single index will handle potential set() method, because it could simply return index 0 and put removed as well as added items.

Having that said it's high time to go with the implementation.

jodator added a commit that referenced this issue Jul 20, 2020
Feature: Introduced the `Collection.addMany()` method for adding multiple items in a single call. Closes #7627.

Feature: Introduced the `Collection.change` event. See #7627.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. package:utils type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants