piotrwitek / typesafe-actions

Typesafe utilities for "action-creators" in Redux / Flux Architecture
https://codesandbox.io/s/github/piotrwitek/typesafe-actions/tree/master/codesandbox
MIT License
2.41k stars 98 forks source link

Reducer ReturnType doesn't work with Redux combineReducers #26

Closed kralcifer-ms closed 6 years ago

kralcifer-ms commented 6 years ago

Could you flush out your example more? Unless I'm missing something, our reducers use combineReducers from Redux a lot and it complains when I try to use ReturnType to save on boilerplate code. Is there a way to get them to play nicely together? complains that A Action isn't same as PayloadAction. Using your new 2.0 stuff and loving it. Thanks for making it!

piotrwitek commented 6 years ago

Hi @kralcifer-ms Please include a full code sample so I could investigate

kralcifer-ms commented 6 years ago

Here's a subset of how I have things set up. We have many more actions, reducers, epics in our app but this is what a small piece of one looks like.

Actions.ts

import { ActionsUnion } from 'typesafe-actions';
import { coreActions } from './coreActions';

const actions = {
    coreActions,
};

export type RootAction = ActionsUnion<typeof actions>;

Reducer.ts

import { combineReducers, ReducersMapObject } from 'redux';
import core, * as fromCore from './coreReducer';

export interface IRootState {
    core: fromCore.ICoreState;
}

const reducers: ReducersMapObject = {
    core,
};

export const rootReducer = combineReducers<IRootState>(reducers);

coreActions.ts

import { buildAction } from 'typesafe-actions';
import { OrientationType } from '../utils/appUtils';

export const orientationChanged = buildAction('CORE_ORIENTATION_CHANGED').fsa((orientation: OrientationType) => ({ orientation }));

export const coreActions = {
    orientationChanged,
};

coreReducer.ts

import { combineReducers } from 'redux';
import { getType } from 'typesafe-actions';
import { RootAction } from '../actions/Actions';
import { orientationChanged } from '../actions/coreActions';
import { OrientationType } from '../utils/appUtils';

export interface ICoreState {
    orientation: OrientationType;
}

const orientation = (state: OrientationType = OrientationType.Portrait, action: RootAction) => {
    let newState: OrientationType;
    if (action.type === getType(orientationChanged)) {
        newState = action.payload.orientation;
    }

    return newState || state;
};

export const core = combineReducers<ICoreState>({
    orientation,
});

export default core;

export const getOrientation = (state: ICoreState) => state.orientation;

I would love to change the orientation reducer to this:

const orientation = (_state: OrientationType = OrientationType.Portrait, action: ReturnType<typeof orientationChanged>) => action.payload.orientation;

but when I do, combineReducers complains:

Argument of type '{ appReady: (state: boolean, action: { type: "Navigation/NAVIGATE"; routeName: string; } | { type...' is not assignable to parameter of type 'ReducersMapObject'. Property 'orientation' is incompatible with index signature. Type '(_state: OrientationType, action: PayloadAction<"CORE_ORIENTATION_CHANGED", { orientation: Orient...' is not assignable to type 'Reducer'. Types of parameters 'action' and 'action' are incompatible. Type 'A' is not assignable to type 'PayloadAction<"CORE_ORIENTATION_CHANGED", { orientation: OrientationType; }>'. Type 'Action' is not assignable to type 'PayloadAction<"CORE_ORIENTATION_CHANGED", { orientation: OrientationType; }>'. Property 'payload' is missing in type 'Action'.

piotrwitek commented 6 years ago

@kralcifer-ms Thanks for the details, one more thing is which version of redux types you're depending on? If you're not sure please refer here: https://github.com/piotrwitek/react-redux-typescript-guide#type-definitions-for-react--redux

The guide and most of my work is Redux v4 types compatible

kralcifer-ms commented 6 years ago

Nice. Looks like I may have a mess there:

"react-redux": "5.0.5",
"redux": "3.7.0",
"@types/react-redux": "4.4.47",

I'll try bumping everything to v4 and see what I get. Thank you!

piotrwitek commented 6 years ago

react-redux shoudn't be an issue in your case please updated redux to 4, and try with a simple buildAction.payload first, because fsa is kinda experimental for the moment

piotrwitek commented 6 years ago

Also here is a working example of combineReducers for the old API: https://github.com/piotrwitek/react-redux-typescript-guide/blob/master/playground/src/redux/todos/reducer.ts

Later I'll try to refactor it to the new API and see how that works If you manage to fix it, I'll appreciate if you can can share your findings

kralcifer-ms commented 6 years ago

Well we've actually had a heated debate at work about the new stuff. People aren't liking the .payload because if it's a number or string you lose the nice variable name that we previously had when we put things inside of a payload object. Hence, the use of fsa as you see above. We're actually having trouble figuring out how to use fsa for the scenarios where we want to pass 2 or 3 variables. I was going to open an issue asking you for advice on that too.

kralcifer-ms commented 6 years ago

The link you sent to todos reducer.ts is exactly what we have been doing but I was wanting to move us to your new stuff.

piotrwitek commented 6 years ago

Wait a second I have something something nice for folks that like multi-argument style as well.

Actually I was thinking to give two kind of styles that are aligned with general accepted styles for entire JS community:

import { action } from 'typesafe-actions';

const createUser = (id: number, name: string) => action('CREATE_USER', { id, name });
createUser(1, 'Piotr')
// type => { type: 'CREATE_USER'; payload: { id: number; name: string } } 
// value => { type: 'CREATE_USER', payload: { id: 1, name: 'Piotr' } });

// advanced example
const fetchUser = {
  request: () => action('FETCH_USER_REQUEST'),
  success: (user: { id: number; name: string }) => action('FETCH_USER_SUCCESS', user),
  failure: (reason: string, traceId: string) => action('FETCH_USER_FAILURE', reason, traceId),
};

fetchUser.request();
// type => { type: 'FETCH_USER_REQUEST' }
// value => { type: 'FETCH_USER_REQUEST' }
fetchUser.success({ id: 1, name: 'Piotr' });
// ...
fetchUser.failure('not found', 'fake_trace_id');
// type =>  { type: 'FETCH_USER_FAILURE'; payload: string; meta: string; } 
// value => { type: 'FETCH_USER_FAILURE', payload: 'not found', meta: 'fake_trace_id' }

Notice that the resulting types are very precise, without optional/undefined values, meta & payload will be present only when they are expected at runtime (completely type-safe).

If you can, please show it to your co-workers at work and tell me what you guys think about it. Cheers!

EDIT: if you want functionality of geType / isActionOf that will work too, only need to wrap creator in simple decorator withTypeMeta(...)

kralcifer-ms commented 6 years ago

looks exactly like what we want. we'd love to try it.

piotrwitek commented 6 years ago

I'll publish a new version later today

piotrwitek commented 6 years ago

Resolved by new version release, please use v2.0.0-rc.0