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

A better way to handle redux action types? #4737

Closed
yourfavorite opened this issue Aug 29, 2017 · 9 comments
Closed

A better way to handle redux action types? #4737

yourfavorite opened this issue Aug 29, 2017 · 9 comments

Comments

@yourfavorite
Copy link

I've been trying to work flow into a react-redux project but getting flow to interact smoothly with Redux actions is proving difficult. The issue is that each reducer needs to be aware of every single action type in order to work without error. This wouldn't be a huge problem for a simple app, but for a larger app, especially one that uses modules like react-router and react-redux-form, we're looking at a lot of different actions that need typing.

Here is an example of a working isolated reducer. This works great by itself, but when put into an app with other reducers, it will error out because right now this reducer only expects one of two action types (LOAD_PREFS_SUCCESS and UPDATE_PREF).

We can get around this error by adding a generic type to our actionType as a catch all. This however produces flow errors as seen here.

Is there a better way to handle action types in flow?

@asolove
Copy link
Contributor

asolove commented Aug 29, 2017

If you can't define a variant for all the actions, maybe try adding a fake extra case to the action variant. That would force you to define a default case in the reducer. Here's one ugly way to do it:

// @flow
const LOAD_PREFS_SUCCESS = 'LOAD_PREFS_SUCCESS';
const UPDATE_PREF = 'UPDATE_PREF';

export type actionType = {
  +type: typeof LOAD_PREFS_SUCCESS,
  prefs: Array<{_id: string, value: any}>
} | {
  +type: typeof UPDATE_PREF,
  id: string,
  value: any
} | {type: "Anything_Else"};

export default (state: {} = {}, action: actionType) => {
  switch (action.type) {
    case LOAD_PREFS_SUCCESS: {
      const newState = {};
      action.prefs.forEach(p => {
        newState[p._id] = p.value;
      });
      return newState;
    }
    case UPDATE_PREF: {
      return { ...state, [action.id]: action.value };
    }
    default:
      return state;
  }
};

@yourfavorite
Copy link
Author

yourfavorite commented Aug 29, 2017

Not pretty, but that seems to produce no errors. Thanks. Going to use the following for now.

export type actionType = {
  +type: typeof LOAD_PREFS_SUCCESS,
  prefs: Array<{_id: string, value: any}>
} | {
  +type: typeof UPDATE_PREF,
  id: string,
  value: any
} | { type: "FLOW_CATCH_ALL" };

@yourfavorite
Copy link
Author

yourfavorite commented Aug 30, 2017

I think I spoke too soon. It appeared to be working earlier but I guess I was looking at the wrong window. When I use the action type above and run my app, I get the following error when the application starts. The first action type being run is @@redux/INIT

Uncaught TypeError: action must be one of: {
  type: string;
  prefs: Array<{
    _id: string;
    value: any;
  }>;
} | {
  type: string;
  id: string;
  value: any;
} | {
  type: "FLOW_CATCH_ALL";
}

Expected: {
  type: string;
  prefs: Array<{
    _id: string;
    value: any;
  }>;
} | {
  type: string;
  id: string;
  value: any;
} | {
  type: "FLOW_CATCH_ALL";
}

Actual: {
  type: string;
}


    at module.exports../app/reducers/prefs.js._flowRuntime2.default.annotate (http://localhost:1212/dist/renderer.dev.js:4432:54)
    at eval (webpack:///./node_modules/redux/es/combineReducers.js?:45:24)
    at Array.forEach (native)
    at assertReducerShape (webpack:///./node_modules/redux/es/combineReducers.js?:43:25)
    at combineReducers (webpack:///./node_modules/redux/es/combineReducers.js?:99:5)
    at Object../app/reducers/index.js (http://localhost:1212/dist/renderer.dev.js:4183:48)
    at __webpack_require__ (http://localhost:1212/dist/renderer.dev.js:671:30)
    at fn (http://localhost:1212/dist/renderer.dev.js:89:20)
    at Object../app/store/configureStore.dev.js (http://localhost:1212/dist/renderer.dev.js:4851:17)
    at __webpack_require__ (http://localhost:1212/dist/renderer.dev.js:671:30)

@yourfavorite yourfavorite reopened this Aug 30, 2017
@asolove
Copy link
Contributor

asolove commented Aug 30, 2017

Oh, looks like you're using flow-runtime. That's going to add an interesting wrinkle to this. I'm not sure it's doable to get both static and runtime checking to work if we can't explicitly specify all the possible actions. Hmm...

@yourfavorite
Copy link
Author

Ah yes, sorry for not clarifying that. While I see value in both, I think static seems to be a cleaner solution (ie - don't have to include generic type: string). I'll probably just remove flow-runtime from the project for now.

I hope there is an easier usage of both static and runtime with redux in the future.

@asolove
Copy link
Contributor

asolove commented Aug 30, 2017

Might also be worth checking if the libraries generating these actions have libdefs that export a type for all the actions they throw. And if not, maybe open an issue or PR to make it happen. Would be nice to be able to say: all the actions from these three libs, plus my custom actions and get solid types.

@asolove asolove closed this as completed Aug 30, 2017
@yourfavorite
Copy link
Author

Thanks to @csprance for the tip, but you can use this as a generic type that will still allow you type check your actions for the given reducer:

type BaseReduxAction = { type: $Subtype<string> };

So in my case the final action type would look like this:

export type actionType = {
  +type: typeof LOAD_PREFS_SUCCESS,
  prefs: Array<{_id: string, value: any}>
} | {
  +type: typeof UPDATE_PREF,
  id: string,
  value: any
} | { type: $Subtype<string> };

Hope this helps someone else running into this problem!

@Hypnosphi
Copy link
Contributor

+type: typeof LOAD_PREFS_SUCCESS

This is actually not different from +type: string. This produces no errors in Flow:

const x = 'X';
const y: typeof x = 'Y';

See #2639

@Hypnosphi
Copy link
Contributor

Workaround is described here:
#2377 (comment)

In your case it should be

const LOAD_PREFS_SUCCESS: 'LOAD_PREFS_SUCCESS' = 'LOAD_PREFS_SUCCESS';
const UPDATE_PREF: 'UPDATE_PREF' = 'UPDATE_PREF';

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

3 participants