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.

Sign up for GitHub

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

Jump to bottom

Add map actions for multiple items #6

Closed
wants to merge 1 commit into from
Closed

Add map actions for multiple items #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,20 @@ A list can be used as a queue or stack. `enqueueActionTypes` and `pushActionType
allIds: [],
},
addActionTypes = [],
addManyActionTypes = [],
changeActionTypes = [],
changeManyActionTypes = [],
removeActionTypes = [],
removeManyActionTypes = [],
keyGetter = action => action.payload.id,
itemGetter = action => ({...action.payload}),
itemModifier = (item, action) => ({...item, ...action.payload}),
addChangeItemsGetter = action => action.payload.reduce((result, item) => ({
...result,
[item.id]: item
}), {}),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*)

itemsModifier = (item, newItem) => ({...item, ...newItem}),
removeKeysGetter = action => action.payload,
resetActionTypes = [],
emptyActionTypes = [],
}
Expand Down
64 changes: 63 additions & 1 deletion src/reducers/__tests__/map.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,32 @@ import map from '../map';
const ACTION_TYPE_1 = 'ACTION_TYPE_1';

const key = 'myId';
const key2 = 'myId2';
const name = 'myName';
const name2 = 'myName2';
const changedName = 'myChangedName';
const changedName2 = 'myChangedName2';

const item = {
id: key,
name: name,
};

const item2 = {
id: key2,
name: name2,
};

const changedItem = {
id: key,
name: changedName,
};

const changedItem2 = {
id: key2,
name: changedName2,
};

const map0 = {
byId: {},
allIds: [],
Expand All @@ -35,6 +48,22 @@ const map1Changed = {
allIds: [key],
};

const map2 = {
byId: {
[key]: {...item},
[key2]: {...item2},
},
allIds: [key, key2],
};

const map2Changed = {
byId: {
[key]: {...changedItem},
[key2]: {...changedItem2},
},
allIds: [key, key2],
};

describe('map', () => {
it('add item', () => {
expect(
Expand All @@ -47,6 +76,17 @@ describe('map', () => {
).toEqual(map1);
});

it('add many items', () => {
expect(
map({
addManyActionTypes: [ACTION_TYPE_1],
})(undefined, {
type: ACTION_TYPE_1,
payload: [item, item2],
})
).toEqual(map2);
});

it('change item', () => {
expect(
map({
Expand All @@ -58,6 +98,17 @@ describe('map', () => {
).toEqual(map1Changed);
});

it('change many items', () => {
expect(
map({
changeManyActionTypes: [ACTION_TYPE_1],
})(map2, {
type: ACTION_TYPE_1,
payload: [changedItem, changedItem2],
})
).toEqual(map2Changed);
});

it('remove item', () => {
expect(
map({
Expand All @@ -68,4 +119,15 @@ describe('map', () => {
})
).toEqual(map0);
});
});

it('remove many items', () => {
expect(
map({
removeManyActionTypes: [ACTION_TYPE_1],
})(map2, {
type: ACTION_TYPE_1,
payload: [key, key2],
})
).toEqual(map0);
});
});
53 changes: 48 additions & 5 deletions src/reducers/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,20 @@ const empty = {
export default ({
initialState = empty,
addActionTypes = [],
addManyActionTypes = [],
changeActionTypes = [],
changeManyActionTypes = [],
removeActionTypes = [],
removeManyActionTypes = [],
keyGetter = action => action.payload.id,
itemGetter = action => ({...action.payload}),
itemModifier = (item, action) => ({...item, ...action.payload}),
addChangeItemsGetter = action => action.payload.reduce((result, item) => ({
...result,
[item.id]: item
}), {}),
itemsModifier = (item, newItem) => ({...item, ...newItem}),
removeKeysGetter = action => action.payload,
resetActionTypes = [],
emptyActionTypes = [],
}) => (
Expand All @@ -20,10 +29,16 @@ export default ({
const {type} = action;
if (addActionTypes.includes(type)) {
return add(state, action, keyGetter, itemGetter);
} else if (addManyActionTypes.includes(type)) {
return addMany(state, action, addChangeItemsGetter);
} else if (changeActionTypes.includes(type)) {
return change(state, action, keyGetter, itemModifier);
} else if (changeManyActionTypes.includes(type)) {
return changeMany(state, action, addChangeItemsGetter, itemsModifier);
} else if (removeActionTypes.includes(type)) {
return remove(state, action, keyGetter);
} else if (removeManyActionTypes.includes(type)) {
return removeMany(state, action, removeKeysGetter);
} else if (resetActionTypes.includes(type)) {
return initialState;
} else if (emptyActionTypes.includes(type)) {
Expand All @@ -33,14 +48,24 @@ export default ({
}
};

const add = (state, action, keyGetter, itemGetter) => {
const key = keyGetter(action);
const addItem = (state, key, item) => {
return {
byId: {...state.byId, [key]: itemGetter(action)},
byId: {...state.byId, [key]: item},
allIds: state.allIds.includes(key) ? state.allIds : state.allIds.concat(key),
};
};

const add = (state, action, keyGetter, itemGetter) => {
return addItem(state, keyGetter(action), itemGetter(action));
};

const addMany = (state, action, addChangeItemsGetter) => {
const items = addChangeItemsGetter(action)
return Object.keys(items).reduce((result, key) => {
return addItem(result, key, items[key]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}, state);
};

const change = (state, action, keyGetter, itemModifier) => {
const key = keyGetter(action);
const item = state.byId[key];
Expand All @@ -50,8 +75,18 @@ const change = (state, action, keyGetter, itemModifier) => {
};
};

const remove = (state, action, keyGetter) => {
const key = keyGetter(action);
const changeMany = (state, action, addChangeItemsGetter, itemsModifier) => {
const items = addChangeItemsGetter(action);
return Object.keys(items).reduce((result, key) => {
const item = result.byId[key];
return item === undefined ? result : {
byId: {...result.byId, [key]: itemsModifier(item, items[key])},
allIds: result.allIds,
};
}, state);
};

const removeItem = (state, key) => {
const item = state.byId[key];
if (item === undefined) return state;
const byId = {...state.byId};
Expand All @@ -61,3 +96,11 @@ const remove = (state, action, keyGetter) => {
allIds: state.allIds.filter(x => x !== key),
};
};

const remove = (state, action, keyGetter) => {
return removeItem(state, keyGetter(action));
};

const removeMany = (state, action, removeKeysGetter) => {
return removeKeysGetter(action).reduce(removeItem, state);
};