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

Consider using packages from redux-utilities #17

Closed
nickmccurdy opened this issue Mar 8, 2018 · 42 comments
Closed

Consider using packages from redux-utilities #17

nickmccurdy opened this issue Mar 8, 2018 · 42 comments
Projects

Comments

@nickmccurdy
Copy link
Collaborator

nickmccurdy commented Mar 8, 2018

The redux-utilities org has some pretty popular packages, it might make sense to depend on them partially. I haven't really used them that much so please voice your opinions.

redux-actions

This seems like a cool abstraction to handling actions, similar to the one we already have. Though if it requires FSAs it's probably not a great idea unless we can get them to patch it upstream to support plain actions.

reduce-reducers

This is a handy way to compose reducers together. I don't think we need it yet but it could help us write more complex high order reducers in the future.

redux-promise

This is one of the more popular async packages along with thunk and saga. I'm fine with redux-thunk, but could there be an advantage to switching to promises or supporting both thunks and promises? Can the thunk package alone support promises and completely replace the need to use redux-promise? I think that's an especially important thing to consider since many async HTTP request libraries use promises now.

@nickmccurdy
Copy link
Collaborator Author

Hm, it seems like redux-thunk supports manual promise handling on the return values of action creators, so I'm fine with not adding redux-promise it it provides a lot of syntax sugar and abstraction over errors and FSAs that we don't want to necessarily force users to use.

@markerikson
Copy link
Collaborator

I've definitely been considering adding something like redux-actions to this lib. I'm not sure whether it's better to DIY that, or pull in the lib.

One reason we might want to DIY it is that the createReducer utility we've already got is basically the same as handleActions() from redux-actions, I think.

For reduce-reducers, we could probably just copy it rather than adding a package. Hardly worth the extra dependency :)

@nickmccurdy
Copy link
Collaborator Author

nickmccurdy commented Mar 9, 2018

Hmm, I guess redux-actions is the only package that would make sense to consider using? It does sound like a good idea, are you aware if it works without FSA?

I was suggesting reduce-reducers more in terms of using it internally than re-exporting it, but I guess we probably won't need it now and we could always pull it in if/when we do.

@markerikson
Copy link
Collaborator

Another related example or source of inspiration is https://github.com/christiangenco/createReducerActions , and the article Namespacing Actions for Redux is also relevant.

@nickmccurdy nickmccurdy added question Further information is requested discussion and removed question Further information is requested labels Mar 21, 2018
@markerikson
Copy link
Collaborator

markerikson commented Apr 7, 2018

Other similar util libs (there's many of them, just listing a couple):

For that matter , https://github.com/cl1ck/redux-bits looks awfully similar to this lib.

@neurosnap
Copy link

I agree about there being overlap between redux-actions and createReducer. The primary innovation of redux-actions is overriding toString of the action, which provides the ability to get rid of the constants action-types altogether.

I'm pretty biased in terms of middleware and prefer to use redux-saga, so long as this library gets out of my way to including it and not using redux-thunk

@neurosnap
Copy link

neurosnap commented Jul 17, 2018

Something as simple as this would work -- i think

export interface Action<T, P> {
  readonly type: T;
  readonly payload: P;
}
type ActionType = string;

function actionCreator<P>(type: ActionType) {
  const action = (payload?: P): Action<ActionType, P> => ({
    type,
    payload,
  });

  action.toString = () => `${type}`;
  return action;
}

export default actionCreator;

@swyxio
Copy link

swyxio commented Aug 18, 2018

hi! im new to this project so pardon if i miss anything but just figured i could help study the existing "actionCreatorCreator" ideas for "reducing action boilerplate" while sitting with acemarke at React Rally.

As a general rule they all involve:

  • making an object with keys as action types doubling as constants,
  • mapping to functions that take a state and optionally a payload

but there's decent variability beyond that.

I really like the way autodux breaks down the types of boilerplate:

  • Action type constants
  • Action creators
  • Reducers
  • Selectors

and a final general nice to have is namespacing (as acemarke linked to above)

as a consequence I really like their approach to solving it haha. so lets do a quick tour of the options acemarke listed:

redux-bits

I am rejecting this out of hand because there is way too much magic going on and would be hard to fit in with other usages of redux (in particular having its own React story with render props instead of being usable with connect). obv feel free to disagree.

redux-actions

redux-actions makes you createActions separately. For reducer creation we map actions to a reducer function. There's no affordance for selectors. it does have a nice combineActions utility.

const { createAction, handleAction } = require('redux-actions');
const increment = createAction('INCREMENT');
const decrement = createAction('DECREMENT');
const reducer = handleActions(
  {
    [increment]: state => ({ ...state, counter: state.counter + 1 }),
    [decrement]: state => ({ ...state, counter: state.counter - 1 })
  },
  defaultState
);
const store = createStore(reducer, defaultState);

// dispatch
store.dispatch(increment()); // or store.dispatch(decrement());

reduxr

reduxr defines actions as part of the reducer object map so you can do things like Todo.action.todoToggleVisibility and Todo.reducer accordingly in appropriate places. this is nice but it might as well do the work of combineReducers going into createStore but doesn't. so basically the object that reduxr creates (aka Todo) does a lot but not enough.

// Todo.js
import { reduxr } from 'reduxr'

// define our reducers in a map
const todoReducer = {
  todoToggleVisibility: (state) => {
    return {
      ...state,
      visible: !state.visible  
    }
  },
  complete: (state, payload) => {
    return {
      ...state,
      complete: payload
    }
  }
}

// define our initial state
const initialState = {
  label: '',
  visible: false
}

// create
const Todo = reduxr(todoReducer, initialState)

// create store
const store = createStore(
  combineReducers({
    todo: Todo.reducer // load our reducer into the store
  })
)

// dispatching
store.dispatch(Todo.action.todoToggleVisibility({visible: true}))

createReducerActions

createReducerActions is basically the same as reduxr, but has a nicer interface to work with react-redux's connect() function:

const mapStateToProps = state => {
  return { counter: state.counter };
};
const mapDispatchToProps = actions; // automatically includes all exported actions

export default connect(mapStateToProps, mapDispatchToProps)(Counter);

autodux

autodux does the above, but also includes namespacing and selectors:

const {
  reducer,
  initial,
  slice,
  actions: {
    increment,
    decrement,
    multiply
  },
  selectors: {
    getValue
  }
} = autodux({
  // the slice of state your reducer controls
  slice: 'counter',

  // The initial value of your reducer state
  initial: 0,

  // No need to implement switching logic -- it's
  // done for you.
  actions: {
    increment: state => state + 1,
    decrement: state => state - 1,
    multiply: (state, payload) => state * payload
  },

  // No need to select the state slice -- it's done for you.
  selectors: {
    getValue: id
  }
});

so using autodux creates a fairly powerful set of creators and selectors and reducers out of the box, and you can further write actions for business logic easily.

i like autodux.

@markerikson
Copy link
Collaborator

Agreed.

I suppose the next question is, what's the best/nicest way to borrow what autodux does, but using our own createReducer implementation so we get that Immer goodness?

autodux is MIT licensed, so it's certainly fair game if we want to start copying (just gotta give appropriate credit).

@nickmccurdy
Copy link
Collaborator Author

Perhaps I'm missing something, but couldn't immer be used around arguments for autodux's create reducer or a store enhancer?

@ChrisLincoln
Copy link

Have you considered adding Kea to mix? "Reduce redux boilerplate" is how I discovered it.

@swyxio
Copy link

swyxio commented Aug 19, 2018

just a quick response that im not at all familiar with how immer works in this package yet so i'll take some time to try to see how this can be implemented. busy being a tourist today!

@nickmccurdy nickmccurdy moved this from In progress to To do in Roadmap Aug 19, 2018
@nickmccurdy
Copy link
Collaborator Author

nickmccurdy commented Aug 19, 2018

@markerikson @sw-yx Do you think we should use Autodux and give up on Selectorator? I'm almost done typing Selectorator in TypeScript, so I would be abandoning my work and typing Autodux instead. I'd just like to know before putting more time into TypeScript support.

@neurosnap
Copy link

neurosnap commented Aug 20, 2018

It seems like the defining features with autodux are 1) combining actions and reducers into one function, 2) auto-creating selectors, 3) preserving the idea of state slices.

To me the real innovation is combining actions and reducers, which I've seen in quite a few other libraries:

Removing the need for defining action types and combining actions into reducers would be a huge win in my mind. Couldn't we accomplish this pretty simply with a reducer enhancer/reducer?

@swyxio
Copy link

swyxio commented Aug 20, 2018

eeps. definitely dont want to sway your opinion @nickmccurdy - i am a visitor here and didn't even see selectorator or the other options. would definitely defer to your judgment here, i was just trying to help.

@markerikson
Copy link
Collaborator

markerikson commented Aug 20, 2018

What I think I'd like is to swipe as much functionality from Autodux as possible, but using our own createReducer implementation for the Immer aspect.

I don't think this interferes with use of Selectorator - that's still useful if someone wants to write selectors themselves, in addition to whatever might get auto-generated.

@neurosnap : I think you're misreading what Autodux does. It doesn't "combine actions and reducers" - it just auto-generates action creators, action types, and selectors, by intelligently deriving things based on the reducer names you provide and the state slice stuff. Those functions work the same as if you'd written those yourself.

But yes, the goal here is that if an end user uses this package, in most situations they wouldn't have to be writing action creators and action types by hand.

@neurosnap
Copy link

neurosnap commented Aug 20, 2018

@markerikson you're right, I might be missing something. It looks like in the source code it is returning a reducer that you can use: https://github.com/ericelliott/autodux/blob/master/src/index.js#L102

At first glance it looks like it will take the action payload and pass it the state. To me this is a way to create an action where its payload is the reducer function. So from my point-of-view this library helps you remove the needs for action types and combines actions and reducers into one concept ... even though under the hood it still preserves the redux concepts of action types, actions, and reducers.

But it all depends on what the goals are for this project. To me it would be worthwhile to explore that idea: removing the need for action types and combining actions and reducers into one concept.

Auto-generating selectors is an interesting idea, but to me seems tertiary to what I described above and can be added at a later time.

@markerikson
Copy link
Collaborator

markerikson commented Aug 20, 2018

@neurosnap : no, that's not what the Autodux code is doing. Let me break it down. Here's the relevant chunk:

    // Look for reducer with top-to-bottom precedence.
    // Fall back to default actions, then undefined.
    // The actions[subType] key can be a function
    // or an object, so we only select the value
    // if it's a function:
    const actionReducer = [
      get(`${ subType }.reducer`, actions),
      actions[subType],
      get(`${ subType }.reducer`, defaultActions)
    ].reduceRight((f, v) => selectFunction(v) || f);

    return (namespace === slice
      && (actions[subType] || defaultActions[subType])
    ) ?
      (actionReducer) ?
        actionReducer(state, payload) :
        Object.assign({}, state, payload) :
      state
;

It's trying to handle several possible ways of associating an action type with the reducer function that should handle that action. Per the docs, it looks like these are all valid:

// Pass reducer functions as keys under the `actions` field
autodux({
    actions : {
        increment: state => state + 1,
    } 
})

// Define both a function for mapping action creator params to the action object,
// and the reducer itself
autodux({
    actions : {
        increment: {
            create : amount => ({amount}),
            reducer : (state, payload) => state + payload.amount
        }
    } 
})

// Or, fall back to the default behavior, which is to assign the provided value directly to that key in the state

So, ultimately all that logic is trying to do is look up the correct reducer function that corresponds with the dispatched action, and call it with reducerFunction(state, payload). Note that because we've associated an action type with a specific reducer function, there's not really a need to pass the entire action object to the reducer - just the payload field with whatever data is being used. This is a fairly common variation on the "Flux Standard Action" approach, as it assumes there's a field called payload, but ignores a potential error field in the action object.

Note that none of this involves putting a "reducer function" into the dispatched action object itself.

Now, I've seen people put "reducer functions" into action objects before. That's always been possible because you can put anything into an action object. That is a bad idea, even if it technically runs, and we discourage people from doing that.

@neurosnap
Copy link

Ah ok, so the actions key in autodux are not actually actions, they are mapping actions being dispatched to a reducer function. I see now where I wasn't thinking about this properly.

In reference to your comments about putting reducer functions inside the action creator, you main point of contention is that functions are not serialized properly?

@markerikson
Copy link
Collaborator

Yes, serializability is the primary reason. The other factor is that in Redux, conceptually a reducer has the responsibility of defining the state's structure and controlling what the update is, in part because you know where to look for how and why your state ended up with its final values. If an actual reducer just does return {...state, ...action.payload}, or even return action.reducer(state), then it's really handing over control of what that state looks like to the code that dispatched the action in the first place. (That also completely assumes that the logic that dispatched the action has actually correctly formatted the data as well.)

Now, there's certainly plenty of times where it's easier and even reasonable to do a "blind merge" reducer like {return ...state, ...action.payload}. But, I try to minimize the number of times I do that - explicit reducer logic makes it clearer how the state is going to change.

@nickmccurdy
Copy link
Collaborator Author

@sw-yx You're didn't do anything wrong, I'm just biased toward selectorator. 😆 Anything similar like autodux's selectors would be fine, just more work to type.

@markerikson Hm, those autodux features sound more useful than I thought originally. If we're not going to include the package, what parts of it would you want to reimplement?

@markerikson
Copy link
Collaborator

The majority of it, I think. The file is only like 150 lines, although it's rather tersely written, and also depends on lodash/fp/get: https://github.com/ericelliott/autodux/blob/7c8b6e78fc3726c3c71c3301491630e90ff5a6c3/src/index.js . I'd prefer not having that as a dependency unless necessary, and I want the reducer generation logic to use our own createReducer instead for the immutability benefits.

It is MIT-licensed, so we could certainly swipe a lot of it with proper attribution.

@neurosnap
Copy link

neurosnap commented Aug 21, 2018

Here's a little MVP I wrote tonight, plenty of room for improvement

Implementation

import createNextState from 'immer';

interface Action {
  type: string;
  payload: any;
}
type ActionCreator = (payload?: any) => Action;
interface ActionMap {
  [key: string]: ActionCreator;
}
type Reduce<State> = (state: State, payload?: any) => State;
interface ReduceMap<State> {
  [key: string]: Reduce<State>;
}
interface ICreate<State> {
  slice: string;
  actions: { [key: string]: Reduce<State> };
  initialState: State;
}

const defaultReducer = <State>(state: State) => state;
const getType = (slice: string, action: string) =>
  slice ? `${slice}/${action}` : action;

export function create<State>({
  slice = '',
  actions = {},
  initialState,
}: ICreate<State>) {
  const actionKeys = Object.keys(actions);

  const reducerMap: ReduceMap<State> = actionKeys.reduce(
    (map: ReduceMap<State>, action: string) => {
      map[getType(slice, action)] = actions[action];
      return map;
    },
    {},
  );

  const reducer = (state: State = initialState, { type, payload }: Action) => {
    const actionReducer = reducerMap[type] || defaultReducer;
    const produce = (draft: State) => actionReducer(draft, payload);
    return createNextState<any>(state, produce);
  };

  const actionMap: ActionMap = actionKeys.reduce(
    (map: ActionMap, action: string) => {
      map[action] = (payload: any) => ({
        type: getType(slice, action),
        payload,
      });

      return map;
    },
    {},
  );

  return {
    actions: actionMap,
    reducer,
    slice,
  };
}

I might have missed something but I couldn't find a good way to use createReducer mainly because it maps the entire action instead of just the payload, like what autodux does.

Usage

import { createStore } from 'redux';
import { create } from './index';

const { reducer, actions } = create<number>({
    actions: {
      increment: (state: any) => state + 1,
      decrement: (state: any) => state - 1,
      multiply: (state: any, payload: any) => state * payload,
    },
    slice: '',
    initialState: 0,
  });

const store = createStore(reducer);
console.log('init', store.getState());
store.dispatch(actions.increment());
console.log('after', store.getState());

Slices also work

import { createStore } from 'redux';
import { create } from './index';

  const counterOne = create<number>({
    actions: {
      increment: (state: number) => state + 1,
      decrement: (state: number) => state - 1,
      multiply: (state: number, payload: number) => state * payload,
    },
    slice: 'counterOne',
    initialState: 0,
  });

  const counterTwo = create<number>({
    actions: {
      increment: (state: number) => state + 1,
      decrement: (state: number) => state - 1,
      multiply: (state: number, payload: number) => state * payload,
    },
    slice: 'counterTwo',
    initialState: 0,
  });

  const rootReducer = combineReducers({
    counterOne: counterOne.reducer,
    counterTwo: counterTwo.reducer,
  });
  const store = createStore(rootReducer);
  console.log('init', store.getState());
  store.dispatch(counterOne.actions.increment());
  store.dispatch(counterTwo.actions.increment());
  store.dispatch(counterTwo.actions.multiply(10));
  console.log('after', store.getState());

@markerikson
Copy link
Collaborator

Hey, neat. About to head to bed, but I'll look at that later.

It's up for debate whether createReducer should pass the entire action to the slice reducers, or just the payload. The version I use in my own app just does payload, but I could see very valid arguments that there might be additional info needed from the action.

@nickmccurdy
Copy link
Collaborator Author

nickmccurdy commented Aug 21, 2018

@neurosnap Looks like a pretty good start, thanks. I'm new to Redux's TypeScript declarations, does that specifically depend on Redux 3 or 4? This package still uses 3 but I got 4 working in #35.

@neurosnap
Copy link

neurosnap commented Aug 21, 2018

I'm not pulling in any typings for redux to get this to work so as far as I know, this will work with both. The main typing is going to be the state which is a generic the end developer will pass into the function.

@markerikson
Copy link
Collaborator

FWIW, per https://twitter.com/acemarke/status/1031756437343674368 , there's a clear majority in favor of passing the entire action to the reducer functions (as normal), rather than just the payload.

@neurosnap
Copy link

neurosnap commented Aug 22, 2018

Nice poll! To go against the grain here if the actions are created by the helper library, I feel like we could avoid passing the entire action through because the concept of an action object would be hidden to the end developer. We are using actions as a vehicle to accomplish our goal, but really our hybrid action + reducer shouldn't need anything more than the payload. It certainly has no reason to have access to the type.

Regardless, I was able to get the MVP to use createReducer and passing the entire action with minor code changes. I'll submit a PR so we don't lose the code and possibly move the discussion there?

@markerikson
Copy link
Collaborator

@neurosnap : it's still possible that there might be additional changes to the actions added by middleware or something. Just something to consider.

@markerikson
Copy link
Collaborator

@neurosnap : wanted to continue our discussion from Reddit. Now that you've put together robodux as a separate lib, how would you want to go about integrating that into redux-starter-kit?

Two observations on the behavior of robodux. I note that you've got a slightly different version of createReducer in there that passes in payload rather than action. Also, I do kinda like the "payload customization" argument that it has, like:

actions: {
  multiply: {
    create: by => by,
    reducer: (state, payload) => state * payload
  }
}

Not sure how complex that would get to reimplement using TS.

Anyway, @nickmccurdy said he'd chatted with you briefly about next steps, but I didn't hear any details. Thoughts?

@neurosnap
Copy link

It's up to you all how we'd like to proceed. I could try to add the payload customization functionality to robodux or the the PR here.

I use createReducer and wrote createAction which robodux uses under the hood to build the actions and reducers.

I created robodux because I had an immediate need to use this functionality with typescript support. I also did not want to include the other dependencies that are getting re-exported in this library.

@markerikson
Copy link
Collaborator

@nickmccurdy , you said you talked to @neurosnap about this some. Was there a conclusion?

Our main options here are:

  • Use robodux as a dependency, tweak it if we're agreed on changes, and re-export it
  • Copy-paste into redux-starter-kit and take ownership of it

@neurosnap
Copy link

Either one works for me. Bringing in robodux would mean this repo does not have control over the code and how it changes over time. I don't anticipate many changes from its current feature-set mainly because it's doing all that I want it to do.

Copy/pasting code would be rather simple for you all to do, especially since I have a PR that does most of the work.

@markerikson
Copy link
Collaborator

@neurosnap : In that case, yes, I'm inclined to really just copy-paste the entire thing except createReducer. I'd be happy to credit you somewhere if you want, although we don't have any particular "contributors list" or anything atm.

I suppose the roadblock atm is that we don't actually have TS support merged in yet ( #38 ), which means we probably couldn't actually compile the TS code from robodux.

@markerikson
Copy link
Collaborator

Okay, experimented with something very briefly this evening. Here's my plan. I've cloned the robodux repo, and fiddled with the TS build settings to spit out ESNext syntax and ES modules. That spits out very nice code, plus the typedef files.

I'll commit those files to this repo, and then modify things enough to use our own createReducer and whatever other tweaks I feel are appropriate, using that JS source.

Once we actually do have #38 finally ready, we can convert the code back to TS and go from there.

I'll try to work on this on Saturday. I'll probably publish redux-starter-kit@0.0.8 first with the current code just to ensure the publish process works with the new package name, then drop in the createSlice stuff and publish like 0.1.0 or something.

@neurosnap
Copy link

What does that method get you over #37?

@markerikson
Copy link
Collaborator

Looks like #37 is less complete than what's in the robodux repo right now, so I'd prefer to go with the current robodux source as the basis.

@neurosnap
Copy link

Sounds good

@markerikson
Copy link
Collaborator

I've just ported createAction, the selector logic, and createSlice, and published redux-starter-kit@0.2.0.

I do note that autodux does several things that robodux doesn't have yet (customizing payloads, more complex selector handling, etc. Might be nice to port some of that functionality as well.

@icopp
Copy link

icopp commented Nov 30, 2018

For utilities, I think that redux-pack would be looking at as an alternative pattern to redux-promise. It uses a meta property on actions to use the same action type for all promise results, with a mini-reducer inside each reducer key to allow handling the different variations of promise behavior (start, success, failure, finish) without expanding on the number of separate action types.

@denisw
Copy link
Contributor

denisw commented Jan 10, 2019

@markerikson Is this issue worth keeping open? A lot has been already incorporated in 0.2.0, and for everything else specific issues probably work better.

@markerikson
Copy link
Collaborator

Yeah, agreed. We've got the baseline functionality in, now the question is what additions do we make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Roadmap
  
Done
Development

No branches or pull requests

7 participants