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

Feature: "add all" action types for maps #5

Open
bsonntag opened this issue Feb 2, 2018 · 5 comments
Open

Feature: "add all" action types for maps #5

bsonntag opened this issue Feb 2, 2018 · 5 comments

Comments

@bsonntag
Copy link

bsonntag commented Feb 2, 2018

Right now there is no simple way to add multiple items at once to a map. We have to dispatch an add action for each element when we want to add a list of items.

It would be nice to have a addAllActionTypes option that would iterate on the action's payload and add all items. This could also need a collectionGetter option.

For lists, since you're using Array.prototype.concat, the add actions work for adding both single and multiple items.

@adrienjt
Copy link
Owner

adrienjt commented Feb 3, 2018

I see your point. Like you said, it's possible to add hundreds of items to a map at once by dispatching many actions. That's exactly what I do. I know it is not the most efficient way, but the performance impact has been negligible in my case, and a little for-loop in the client code isn't a big deal for me. It's kept the map API simple.

Now, I could see a situation where you already have an action with several items, which is reduced by another reducer, and you'd like to reduce that same action into a map, without having to dispatch new actions. Are you in that situation?

I can't promise that I'll implement this soon, so feel free to submit a PR. It would be greatly appreciated. I'd call it addMany... rather than addAll..., and I would also include a changeMany... and removeMany.... Also, how would you get the keys of the collection items?

Thank you for using redux-data-structures!

@bsonntag
Copy link
Author

bsonntag commented Feb 5, 2018

Well, I'm using redux-promise-middleware for API requests. I get a list of entities on the fulfilled action that I want to add to a map. I still want to dispatch the fulfilled action because of some request state reducers (isFetching and stuff like that) and it would be nice if that would be enough to add all entities to the map.

Also, how would you get the keys of the collection items?

I was thinking that keyGetter would help with that, but its default is action => action.payload.id, so that won't work. We'd need collectionKeyGetter (item => item.id by default) and maybe even collectionItemGetter (item => ({ ...item }) by default).

Example:

const users = map({
  addActionTypes: ['CREATE_USER_FULFILLED'],
  addManyActionTypes: ['FETCH_USERS_FULFILLED'],
  changeActionTypes: ['UPDATE_USER_FULFILLED'],
  removeActionTypes: ['DELETE_USER_FULFILLED'],
  collectionGetter: action => [...action.payload], // default
  collectionItemGetter: item => ({ ...item }), // default
  collectionKeyGetter: item => item.id, // default
  itemGetter: action => ({ ...action.payload }), // default
  itemModifier: (item, action) => ({ ...item, ...action.payload }), // default
  keyGetter: action => action.payload.id, // default
});

I'll submit a PR for this when I have time.

@bsonntag
Copy link
Author

bsonntag commented Feb 5, 2018

@adrienjt changeMany... isn't as easy as I first thought, because itemModifier takes the action as it's second argument. I'll have to add another option for modifying items from a collection.

What do you think about changing itemModifier to receive the result of itemGetter instead of action? Like this: itemModifier = (item, newItem) => ({ ...item, ...newItem }). This way I could easily implement changeMany... without adding another option. This would be a breaking change, though...

@adrienjt
Copy link
Owner

adrienjt commented Feb 7, 2018

Let's create an itemsModifier (note the plural) with your proposed signature for now, and I'll merge it with itemModifier in 1.0.0.

@rsayn
Copy link

rsayn commented Sep 6, 2018

I had the same issue: I solved it by using the reduce-reducers library to create a flat-combined reducer with the map reducer and a custom reducer for multiple add/remove/replace handling.

Code

import { map } from "redux-data-structures";
import reduceReducers from "reduce-reducers";

const reducer = reduceReducers(map({...}), replaceAllReducer, addAllReducer, removeAllReducer);

By doing this, the "all" reducers inherit the state shape from map, obtaining the visibility they need to operate on byId and allIds.

It's a workaround, but it works for the time being.

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 a pull request may close this issue.

3 participants