Add map actions for multiple items

[bsonntag]

This adds addMany... and removeMany... actions to map. These actions can be used to add multiple items to the map with only one action.

For this, three other options where added:

• collectionGetter = action => [...action.payload]
• collectionItemGetter = item => ({ ...item })
• collectionKeyGetter = item => item.id

Closes #5.

[bsonntag]

I couldn't run the tests. They kept failing with:

SyntaxError: Unexpected token import

Even after I installed babel-jest and updated jest they failed with:

TypeError: Path must be a string. Received undefined.

[adrienjt]

Hi Benjamin,

Thanks a lot for submitting this PR!

• RE running the tests
  That's weird. Maybe it's a jest version mismatch between the one in devDependencies and the one you (or I) might have installed globally. I'll check on my side tomorrow.

• RE the API design
  In my opinion, using the same collectionKeyGetter for both addMany and removeMany puts too much constraint on the payload of the removeMany actions. In your test, the payload to remove two items is [{id: key}, {id: key2}], when in most cases it'd probably look like [key, key2].
  I need to think about it a bit more (tomorrow), but it looks like we might have to separate the two key getters. What do you think?

[adrienjt]

@bsonntag, tests were broken when I split the babelrc into two environments, to fix #4.

736db95 should make tests run again.

[adrienjt]

Regarding the design, how about this:

addChangeItemsGetter = action => action.payload.reduce((a, e) => {a[e.id] = e; return a;}, {})
removeKeysGetter = action => action.payload

• addChangeItemsGetter should return an object mapping keys to new or modified items, like {id0: item0, id1: item1}. The default getter creates it from an array of items whose keys are the id property.
• removeKeysGetter should return an array of keys to delete, like [id0, id1].

What do you think?

[bsonntag]

@adrienjt changes applied.

736db95 should make tests run again.

That fixed the tests, thanks.

I don't quite like the addChangeItemsGetter name, but I don't have a better idea...
Also, I'm afraid that itemModifier and itemsModifier will confuse people.

[adrienjt]

This is an unnecessary use of immutability: the accumulator's scope inside Array.prototype.reduce's callback is well defined. We can safely mutate it. The immutable solution is actually very slow compared to the mutable solution I suggested: https://jsperf.com/mutable-vs-immutable-reduce

I know that redux-data-structures doesn't focus on performance (https://github.com/adrienjt/redux-data-structures#performance), but this is a low hanging fruit.

If you're interested in the details, I suggest running the two snippets with node --prof. You'll see that the immutable solution spends most of its time copying arrays:

 [Summary]:
   ticks  total  nonlib   name
    104    0.1%    0.1%  JavaScript
  77095   85.7%   85.9%  C++
    242    0.3%    0.3%  GC
    179    0.2%          Shared libraries
  12573   14.0%          Unaccounted

 [C++ entry points]:
   ticks    cpp   total   name
  76917   99.9%   85.5%  T v8::internal::Runtime_CopyDataProperties(int, v8::internal::Object**, v8::internal::Isolate*)

[adrienjt]

I like your attempt at factorizing some code here. But the performance impact is O(n). Now that addChangeItemsGetter returns the items as a map-Object, you can merge them all at once:

const addMany = (state, action, addChangeItemsGetter) => {
  const items = addChangeItemsGetter(action);
  const keys = Object.keys(items);
  return {
    byId: {...state.byId, ...items},
    allIds: [...new Set(state.allIds.concat(keys))],
  };
}

changeMany and removeMany could use similar optimizations.

[adrienjt]

I don't quite like the addChangeItemsGetter name, but I don't have a better idea...
Also, I'm afraid that itemModifier and itemsModifier will confuse people.

How about manyItemsGetter, manyItemsModifier, and manyKeysGetter?

[bsonntag]

I'm closing this as I no longer use this module so don't have the need for this.
Thank you for your time and sorry for leaving this open and unattended for so long.