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

Creating action creators in Flow, could actions that are classes be supported? #1893

Closed
kasperpeulen opened this issue Aug 11, 2016 · 10 comments

Comments

@kasperpeulen
Copy link

kasperpeulen commented Aug 11, 2016

I've been playing around with react redux and flow in some projects. I like it, but there is one thing I hate: adding an action creator. The problem is, that I have to change stuff, in 3 files, and it is very repetitive. I will show an example:

src/ActionModal/actions.js

import ActionTypes from "./ActionTypes";

export type ShowActionModal = {
  type: 'ShowActionModal'
}

function show(): ShowActionModal {
  return {
    type: ActionTypes.show
  }
}

export type HideActionModal = {
  type: 'HideActionModal'
}

function hide(): HideActionModal {
  return {
    type: ActionTypes.hide
  }
}

export default {show, hide};

src/ActionModal/ActionTypes.js

const show = 'ActionModal/show';
const hide = 'ActionModal/hide';

export default {show, hide};

src/Action.js

import {Increment, Decrement} from './Counter/actions';
import {ShowActionModal, HideActionModal} from './ActionModal/actions';

export type Action = Increment | Decrement | ShowActionModal | HideActionModal;

I have to type 4 times the word ShowActionModal, just to add one simple action creator to my project. This just seems like so much work, for just two simple actions.

I'm wondering if you guys have thought about supporting the following pattern:

src/ActionModal/actions.js

class ShowActionModal extends Action {}
class HideActionModal extends Action {}

src/Action.js

class Action {
  constructor() {    
    // here you can for example log that an action is created ...
    // or nothing :-)
  }
}

This is much less code. There is no need for hardcoding an action constant. And now you can just change one file, anytime you create an action creator (the constructor is now the action creator). There is no need to declare a type, because a class is a type.

The reducer would look like this:

function showModalReducer(state, action) {
  switch (action.constructor) {
    case ShowActionModal:
      return true;
    case HideActionModal:
      return false;
    default:
      return state;
  }
}

Also if you would have some more complex actions, you can do this like this of course:

class ToggleTodo extends Action {
  id: number;
  constructor(id: number){ this.id = id; }
}

Tagging @gcanti as I saw that he has some experience with using redux with flow.

@markerikson
Copy link
Contributor

This is not a scenario that will be directly supported in Redux as far as I know. There are, however, a number of community-built libs that implement more OOP-style approaches like this. See https://github.com/markerikson/redux-ecosystem-links/blob/master/variations.md for links to some of them.

@kasperpeulen
Copy link
Author

@markerikson Okay, I see, but I hoped that this could be considered as this would help a lot with reducing boilerplate in the flow case, that I heard @gaearon wants to have good support for.

I just found out that I could implement this myself in this way:

  1. Don't use createStore from redux library
  2. Copy the createStore file from redux, and remove the following lines:
    if (!isPlainObject(action)) {
      throw new Error(
        'Actions must be plain objects. ' +
        'Use custom middleware for async actions.'
      )
    }

    if (typeof action.type === 'undefined') {
      throw new Error(
        'Actions may not have an undefined "type" property. ' +
        'Have you misspelled a constant?'
      )
    }
  1. Rewrite the object { type: ActionTypes.INIT } as a class
  2. Use this new createStore function in your redux app

@markerikson
Copy link
Contributor

I can't speak to the Flow aspect, but I know Dan has said a number of times that "reducing boilerplate" is not an objective for Redux, and that if you really want to you can build your own abstractions. See https://twitter.com/dan_abramov/status/733742952657342464, http://redux.js.org/docs/recipes/ReducingBoilerplate.html , and #1171 (comment) for some thoughts on that topic.

@kasperpeulen
Copy link
Author

kasperpeulen commented Aug 11, 2016

Dan has said a number of times that "reducing boilerplate" is not an objective for Redux

I understand and appreciate that way of thinking. However, have you read my example. Maybe I should make my example shorter, to make the amount of boilerplate more clear in flow:

// file 1
import ActionTypes from "./ActionTypes";

export type ShowActionModal = {
  type: 'ShowActionModal'
}

function showActionModal(): ShowActionModal {
  return {
    type: ActionTypes.ShowActionModal
  }
}

// file 2
export const ShowActionModal = 'ShowActionModal';

// file 3
import type {ShowActionModal} from './ActionModal/actions';

export type Action = ... | ShowActionModal

actually, if you think about it like this, this is 9 time writing the same word, in 3 different files, to finally get an action creator working. I think the flow case is quite much worse, than the normal case.

So if you would support action to be a class (maybe only support classes without methods, so that they are serializable?), you can reduce all this boilerplate, to the following line:

import Action from '../Action';

class ShowActionModal extends Action{}

I will read a bit more about the links you send me about the concerns about classes. I'm wondering, is this all about, that classes are not serialisable? Is that the concern? Because I don't want a class because I want to add fancy methods to my action, I just want my Action to have only serializable properties. The only reason I like the class, is because it saves me from writing 9 time the same word, in 3 different files.

@markerikson
Copy link
Contributor

Yeah, that's the biggest factor. The biggest reason for Redux's existence is to be able to serialize state and actions to enable time-travel debugging. The vast majority of how it's used falls out from that (plain objectx for actions, with type fields that use strings as constants, etc). Class instances are not serializable, and doing instanceof checks gets really hairy.

Like I said, you're certainly welcome to come up with your own abstractions on top of Redux, but the core is not likely to be changed much at this point, and definitely not for something like this (see #1813 for an example of this type of discussion and Dan's thinking). There are other libs out there that put an OOP-type abstraction on top of actions and reducers, in the list I linked. MobX is also a lot more object-oriented, and generally regarded to have less boilerplate involved.

Ultimately, the main goals for Redux are things like predictable / functional-style state updates, and enabling scenarios like time-travel debugging. Aspects such as serializability and some of the "boilerplate" do generally fall out as results of those goals.

@kasperpeulen
Copy link
Author

kasperpeulen commented Aug 12, 2016

@markerikson

The biggest reason for Redux's existence is to be able to serialize state and actions to enable time-travel debugging. The vast majority of how it's used falls out from that (plain objectx for actions, with type fields that use strings as constants, etc). Class instances are not serializable, and doing instanceof checks gets really hairy.

Okay, well, fair enough, I'm not really convinced that using classes that behave like plain objects, would make time travelling impossible. But that is probably my lack of understanding.

Thanks for the links to MobX etc. but I like redux a lot. I like the time travelling, I just want to get flow support without feeling like writing redundant code all the time.

I actually found a way to do this, while still dispatching plain objects.

Using this class:

export default class Action {
  type: string;

  constructor() {
    this.type = this.constructor.name;
  }
}

Then override the dispatch method of the store

let oldDispatch = store.dispatch;
store.dispatch = (action) => {
  if (action instanceof Action) {
    action = {...action}
  }
  oldDispatch(action);
};

Then write actions like this:

class Increment extends Action {}
class Decrement extends Action {}

And reducers like this:

export default function reducer(counter: number = 0, action: Action): number {
  switch (action.type) {
    case Increment.name:
      return counter + 1;
    case Decrement.name:
      return counter - 1;
    default:
      return counter;
  }
}

I think this is good enough for me, thank you for your time @markerikson

@jimbolla
Copy link
Contributor

What about a middleware that turns a class-based action into a plain object action where the type property is the class name? Then you could define your actions as classes, but redux core would be satisfied.

@kasperpeulen
Copy link
Author

kasperpeulen commented Aug 12, 2016

@jimbolla Thanks for the tip. Yes, this actually works pretty good.

I do it now like this, in the middleware, I convert the action class object, to an plain object.
And I wrap the rootReducer into a function that wraps the plain action objects back to to the class objects.

Works fine :)

export const store = createStore(
  convertPlainObjectToAction(rootReducer, Actions),
  {},
  compose(
    applyMiddleware(convertActionToPlainObject(Action)),
    window.devToolsExtension ? window.devToolsExtension() : f => f
  )
);

function convertPlainObjectToAction(reducer: Reducer<*, *>, Actions: Object): Reducer<*, *> {
  return (state, action) => {
    if (Actions.hasOwnProperty(action.type)) {
      const classAction = new (Actions[action.type])();
      return reducer(state, classAction);
    }
    return reducer(state, action);
  }
}

function convertActionToPlainObject(Action: Class<*>) {
  return () => {
    return (next) => (action) => {
      if (action instanceof Action) {
        action = {...action}
      }
      return next(action);
    }
  }
}

The Action class I define so that it has a type property:

export default class Action {
  type: string;

  constructor() {
    this.type = this.constructor.name;
  }
}

@gcanti
Copy link
Contributor

gcanti commented Aug 12, 2016

@kasperpeulen with flow in place you may want to discard constants (replaced by flow type checking), so no ActionTypes file.
IMO actions creators should be defined only when strictly necessary (for example precisely in order to reduce the boilerplate) not something that you always do.

However flow introduces some boilerplate in its own, a price that I'm glad to pay if you ask me.

Note how you are losing type safety here

class Action {
  type: string;

  constructor() {
    this.type = this.constructor.name
  }
}

class Increment extends Action {
  step: number
}

export function reducer(counter: number = 0, action: Action): number {
  switch (action.type) {
    case Increment.name:
      return counter + action.step; // <= property `step`. Property not found in Action
    case Decrement.name:
      return counter - 1
    default:
      return counter
  }
}

@kasperpeulen
Copy link
Author

kasperpeulen commented Aug 12, 2016

@gcanti

with flow in place you may want to discard constants (replaced by flow type checking), so no ActionTypes file.

Yes, you have a point about the action types. The downside is that you miss the speed and explorability feature of auto completion.

IMO actions creators should be defined only when strictly necessary (for example precisely in order to reduce the boilerplate) not something that you always do.

Yes, I have been wondering about that too. I just create action creators, because people say that this is the way to go. But you mean just dispatching the literal object itself instead?

Note how you are losing type safety here

Yes, I just noted. I have now another solution that solves this and I think is kind of neat:

store.js

import {compose, createStore, applyMiddleware} from 'redux';
import rootReducer from './reducer';
import Action from './Action';
import Actions from './Actions';

const convertPlainObjectToAction = createStore => reducer =>
  createStore((state, action) => {
    if (Actions.hasOwnProperty(action.type)) {
      const constructor = Actions[action.type];
      const newAction = new constructor();
      for (const key of Object.keys(action)) newAction[key] = action[key]
      return reducer(state, newAction);
    } else {
      return reducer(state, action);
    }
  });

const convertActionToPlainObject = () => next => action =>
  (action instanceof Action)
    ? next({...action})
    : next(action);

export const store = createStore(
  rootReducer,
  {},
  compose(
    convertPlainObjectToAction,
    applyMiddleware(convertActionToPlainObject),
    window.devToolsExtension ? window.devToolsExtension() : f => f
  )
);

Action.js

// @flow
export default class Action {
  type: string;

  constructor() {
    this.type = this.constructor.name;
  }
}

Actions.js

// @flow
import Action from '../action';

class Increment extends Action {}
class Decrement extends Action {}

export default {Increment, Decrement};

Dispatch like this: dispatch(new Actions.Increment())

And the reducer is like this:

export default function reducer(counter: number = 0, action: Action): number {
  if (action instanceof Actions.Increment) {
    return counter + 1;
  } else if (action instanceof Actions.Decrement) {
    return counter - 1;
  } else {
    return counter;
  }
}

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

No branches or pull requests

4 participants