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

Add createReducer - a typesafe reducer factory using object map (including getType() support) #106

Closed
MasGaNo opened this issue Jan 29, 2019 · 26 comments · Fixed by #138
Closed
Assignees
Labels

Comments

@MasGaNo
Copy link

MasGaNo commented Jan 29, 2019

Issuehunt badges

First thank you for this project, it's very cool and help us to reduce dramatically the number of files, lines of code, increase the readability and enforce the definition 👍

Is your feature request related to a problem?

In some of our projects we have very big reducers with big switch statement.
So that's why I want to have this kind of approach from Redux documentations:

export const todos = createReducer([], {
  [ActionTypes.ADD_TODO]: (state, action) => {
    const text = action.text.trim()
    return [...state, text]
  }
})
//
function createReducer(initialState, handlers) {
  return function reducer(state = initialState, action) {
    if (handlers.hasOwnProperty(action.type)) {
      return handlers[action.type](state, action)
    } else {
      return state
    }
  }
}

This approach helps us to reduce the cyclomatic complexity of the file, and allow us to test easily each reducer.

Describe a solution you'd like

In the same way I want to enforce the definition based on typesafe-actions, I want to have a way to create the reducers based on the list of actions created with the different createAction method and to get the correct state and action on each reducer.

Who does this impact? Who is this for?

Well... for everybody I guess.

Describe alternatives you've considered

I made a try, but it wasn't fully a success... So if you can help me to complete this approach and/or to improve it, it will be great.
To illustrate my purpose, I prepared a demo-project here. I have 2 main concerns:

  • create-reducers.ts: I get a transpilation error. It's not necessary related to typesafe-actions, so I will continue to investigate.
  • reducers.ts: My goal is to remove the constant approach. So it work like a charm with constant even if I created the actions with createStandardAction, but if I try to deal with getType, the magic disappear.

Additional context

I know that I definitely need to improve my skills about Advanced Definition, so probably, type definition defined in the create-reducers.ts file can easily be improved to manage the different use-case.

Don't hesitate to tell me if you find this discussion appropriate or not here.

Thanks.


IssueHunt Summary

piotrwitek piotrwitek has been rewarded.

Backers (Total: $50.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@MasGaNo
Copy link
Author

MasGaNo commented Jan 29, 2019

Ok good small-news: I fix the error on the create-reducers.ts part by enforcing the definition and removing the any part f779b6d

@piotrwitek
Copy link
Owner

piotrwitek commented Feb 4, 2019

Hey, I suppose this issue is related right? piotrwitek/react-redux-typescript-guide#121

So you want to include createReducer function into the library right?

I implemented it but there is an issue with computed properties assertions which I believe is a bug in TypeScript.

Maybe you can investigate it further in TypeScript repo issue as I want to focus on clearing out the technical debt before adding new features.

Implemented here: piotrwitek/react-redux-typescript-guide@6502fd4#diff-8ed224eb6b7ff91fd900f963dc88b328

screenshot 2019-02-04 at 1 41 24 am

@MasGaNo
Copy link
Author

MasGaNo commented Feb 4, 2019

Ah true! I didn't see this issue in your other repository.

That's indeed the idea for the moment (I have another suggestion to improve typesafe-actions, but probably need an improvement from the TypeScript team).

Ok, I don't know if you already check my repository, but we have more or less the same problem.

If I use the full path string constant as a key, it works like a charn, but if I use getType, there are some errors.

@piotrwitek
Copy link
Owner

piotrwitek commented Feb 4, 2019

Yeah I agree, from the example above I think the problem is not in the getType, but IMO it is a type assertion bug in TypeScript. Please check in TypeScript repo.

@MasGaNo
Copy link
Author

MasGaNo commented Feb 4, 2019

Yes, I'm trying to figure out a small test example for them 👍

@MasGaNo
Copy link
Author

MasGaNo commented Feb 4, 2019

Ah! nice 🙂

I will post it right now in there repository 👍

@piotrwitek
Copy link
Owner

Thanks a lot @MasGaNo! Please link to this issue so we can track it here.

@MasGaNo
Copy link
Author

MasGaNo commented Feb 4, 2019

Here we go: microsoft/TypeScript#29718

@piotrwitek piotrwitek changed the title Splitting big switch case structure in the reducers creation Add a typesafe reducer factory using object map - createReducer Feb 11, 2019
@piotrwitek
Copy link
Owner

Blocked by TypeScript bug, we'll get back to this idea when it's fixed upstream.

@MasGaNo
Copy link
Author

MasGaNo commented Feb 11, 2019

Well, I hope it will be really fixed for TypeScript 3.4.0, as they are currently already working on the 3.3.3 (btw, almost complete).

Wait&see.

For the moment, there is still the workaround with constant, so it remains a good progress against the big switch statement.

@rifler
Copy link

rifler commented Feb 15, 2019

a convenient example from redux-act

import { createReducer } from 'redux-act';
import * as act from './actions';

const someReducer = createReducer<SomeState>((on) => {
	on(act.someAction1, (state, initialOptions) => {
		return {
			...state
		};
	});
    on(act.someAction2, (state, selectedFilterId) => {
        return selectedFilterId !== state.selectedFilterId
            ? {
                  ...state,
                  selectedFilterId,
              }
            : state;
    });
});

but not type safe :( (or I failed)

@MasGaNo
Copy link
Author

MasGaNo commented Feb 18, 2019

Hi @rifler
It's indeed an interested approach.

But personally, I prefer to keep the ObjectMap approach because I like the idea to have my IDE tells me what are the non-implemented reducer, and help me to avoid duplicate redefinition.

Here I don't know if I overwrite the previous implementation or if it will be extended.

At least with this kind of implementation could be more useful IMHO:

type ReducerRegisterGlob<T> = {handlerRegister: ReducerRegister<T>};
type ReducerRegister<T> = <P extends T>(value: P, callback: Function) => ReducerRegisterGlob<Exclude<T, P>>;

function createReducers<T>(callback: (handlerRegister: ReducerRegister<T>) => void) {
    const reducers: any = {};

    function getNext<R>(): ReducerRegister<R> {
        return <P extends R>(value: P, callback: Function) => {
            reducers[value] = callback;
            return getNextGlob<Exclude<R, P>>();
        }
    }

    function getNextGlob<R>() {
        return {
            handlerRegister: getNext<R>()
        };
    }

    callback(getNext<T>());

    return reducers;
}

const enum Actions {
    Action1 = 'action1',
    Action2 = 'action2'
};

const reducers = createReducers<Actions>((handlerRegister) => {
    handlerRegister(Actions.Action1, () => {}) // 'action1'|'action2'
    .handlerRegister(Actions.Action2, () => {}) // 'action2'
    .handlerRegister(); // Error -> 'never' 
});

image

But you still cannot avoid your developer to perform something like:

const reducers = createReducers<Actions>((handlerRegister) => {
    handlerRegister(Actions.Action1, () => {}); // 'action1'|'action2'
    handlerRegister(Actions.Action2, () => {}); // 'action1'|'action2' again
    handlerRegister(Actions.Action1, () => {}); // 'action1'|'action2' again
});

@MasGaNo
Copy link
Author

MasGaNo commented Feb 18, 2019

Or... If you really want to constraint your developer, you can always use this approach:

type NoNever<T, A> = T extends never ? never : A;
type _ReducerRegister<T> = {
    register: NoNever<T, <P extends T>(value: P, callback: Function) => ReducerRegister<Exclude<T, P>>>;
    commit: () => any;
};
type ReducerRegister<T> = OmitByValue<_ReducerRegister<T>, never>;

function createReducers<T>() {
    const reducers: any = {};

    const commit = () => {
        return reducers;
    };

    function getNext<R>(): ReducerRegister<R> {
        return {
            // @ts-ignore
            register: <P extends R>(value: P, callback: Function) => {
                reducers[value] = callback;
                return getNext<Exclude<R, P>>();
            },
            commit
        };
    }

    return getNext<T>();
}

const enum Actions {
    Action1 = 'action1',
    Action2 = 'action2'
};

const reducers = createReducers<Actions>()
    .register(Actions.Action1, () => { }) // 'action1'|'action2'
    .register(Actions.Action2, () => { }) // 'action2'
    .commit(); // register is not available anymore

Here you can avoid at least the duplicate registration of your action.
But it's definitely more verbose. I'm still looking for the Map approach 🙂

@IssueHuntBot
Copy link

@IssueHunt has funded $50.00 to this issue.


@joefiorini
Copy link

joefiorini commented Apr 16, 2019

@piotrwitek et. al

I was able to find an approach that seems to work using a conditional type for the ReducerMap. I've done some basic testing against a reducer with 8 actions. I'm exclusively using PayloadActions in this reducer, so that's what you see here. I could expand this to support other action types in the branches of the conditionals (kinda ugly, maybe there's a nicer way to handle that). I'll open a PR with this, but would appreciate thoughts esp. on how to get rid of the any typecast that is also in the example posted above.

type Action = ActionType<typeof actions>;
type ReducerMap<S, A> = A extends PayloadAction<infer T, infer P>
  ? { [key in T]: (state: S, action: PayloadAction<T, P>) => S }
  : never;

function createReducer<S, A extends { type: string }>(
  initialState: S,
  handlers: ReducerMap<State, Action>,
) {
  return function reducer(state = initialState, action: A): S {
    if (handlers.hasOwnProperty(action.type)) {
      return (handlers as any)[action.type](state, action);
    } else {
      return state;
    }
  };
}

EDIT for some context: This accomplishes linking the specific action type key to the specific action, therefore in your reducer map object it will automatically differentiate, eg., payload types. The tradeoff is that the ReducerMap type definition will have to enumerate each type of action supported. I can't really see a way around that.

EDIT 2: @piotrwitek I guess I should ask before assuming... do you want the ReducerMap type in this library or are trying to keep reducer-related types out of this?

@MasGaNo
Copy link
Author

MasGaNo commented Apr 17, 2019

Hey @joefiorini 🙂

I don't know why you need the any key word, in my implementation, I really don't need it:

/**
 * Create a reducers from a defined handlerMap
 * @param initialState 
 * @param handlersMap 
 */
export function createReducers<T extends ActionType<ActionCreator<StringType>>, S>(
    initialState: S,
    handlersMap: {
        [P in GetActionType<T>]?: ActionReducer<S, T, P>;
    }
) {
    return function <P extends GetActionType<T>>(state: S = initialState, action: GetAction<T, P>) {
        if (handlersMap.hasOwnProperty(action.type)) {
            return handlersMap[action.type]!(state, action);
        }
        return state;
    }
}

image

Then, this solution works perfectly as soon as you specify explicitly the type of your action:

createReducers<ExampleStoreActionsType, ExampleStoreState>(ExampleStoreState, {
    "Example/ActionWithPayload": (state, action) => {
        action.payload.username
        return {
            ...state,
            username: action.payload.username,
            password: action.payload.password
        };
    }
});

image

But the thing is: I really don't like the constant approach 🙂
And that's precisely the concern of this issue, I want to have the action-helper approach instead:

createReducers<ExampleStoreActionsType, ExampleStoreState>(ExampleStoreState, {
    [getType(ExampleStoreActions.ActionWithPayload)]: (state, action) => {
        action.payload.username
        return {
            ...state,
            username: action.payload.username,
            password: action.payload.password
        };
    }
});

And here, it doesn't work:
image

But it's because of a limitation of TypeScript. That's why we created an issue in their side: microsoft/TypeScript#29718

So don't hesitate to upvote, and share this issue with lot of people, as the TypeScript team can reconsider the priority of this feature.

@piotrwitek
Copy link
Owner

@joefiorini I have tested your implementation but it doesn't fix the issues we had with my previous createReducers implementation that I did here: #106 (comment)

Could you tell us what do you think are the benefits of your solution and how it is relevant for this library? If action helpers still don't work (as @MasGaNo mentionted) which is an actual deal breaker here.

@joefiorini
Copy link

@piotrwitek Totally missed the goal of having it work with the getType helper. Sorry about that. Looking back it's clearly spelled out in the first comment, I guess I didn't totally understand the core issue at first. Sorry about that!

This solves my biggest problem which is having the exact payload type inside each reducer map function (the types in redux-starter-kit did not do this). I'm personally okay with the constant approach since I still get a type error if I spell the type name wrong.

I'm going to try out @MasGaNo's solution in my app, since that seems a bit simpler than mine, and if it solves the same problems I'll use that.

@joefiorini
Copy link

@piotrwitek To help clear up anyone else getting confused about this issue, what do you think of renaming it to something along the lines of "Support getType helper in typesafe reducer factories" since this is about more than just "adding a typesafe reducer factory using object map"?

@piotrwitek piotrwitek self-assigned this Apr 18, 2019
@piotrwitek piotrwitek changed the title Add a typesafe reducer factory using object map - createReducer Add a typesafe reducer factory using object map - createReducer (including getType() support) Apr 18, 2019
@piotrwitek piotrwitek changed the title Add a typesafe reducer factory using object map - createReducer (including getType() support) Add createReducer - a typesafe reducer factory using object map (including getType() support) Apr 18, 2019
@piotrwitek
Copy link
Owner

piotrwitek commented Apr 18, 2019

I have a new API proposal ready for this feature, here are the highlights of new main features:

import * as actions from './'

// NEW FEATURE: extend internal typesafe-actions RootAction type 
// so you never have to use generic types in your application 😮 
// it's as simple as below one-liner
declare module 'typesafe-actions' {
  export type RootAction = ActionType<typeof actions>;
}

// now just import helper functions from "typesafe-actions" as always
import { createReducer, getType } from 'typesafe-actions'

const initialState = 0;

// Notice here you don't need to provide Action type to createReducer (magic!),
// because it's derived from the above declare section 😉 
export const counterReducer = createReducer(initialState)
  // you can pass single or multiple action [using array] creators instances
  // and the action arg will be constrained in reducer function
  .handleAction(actions.add, (state, action) => {
    return state;
  })
  // it also works with type constants, here 'ADD' constant will not be accepted because it's handled above (removing handled actions from accepted argument)
  .handleAction('INCREMENT', (state, action) => {
    return state + 1;
  })
  //.handleAction <= exshausive checking! you cannot use "handleAction" anymore because all available actions are handled


counterReducer(0, increment()); // => 1
counterReducer(0, add(4)); // => 4

@piotrwitek
Copy link
Owner

piotrwitek commented Apr 21, 2019

Extended proposal with a new feature to create reducer factory using object map, you can even compose different reducers together with type complete safety:

const counterReducer = createReducer(initialState)
  .handleAction(add, (state, action) => state + action.payload)
  .handleAction(increment, (state, _) => state + 1);

const rootReducer = createReducer(initialState, {
 ADD: counterReducer.reducers.ADD,
 INCREMENT: counterReducer.reducers.INCREMENT,
})

All is already implemented and extensively tested, so should be rock solid and production ready.

I just need to add documentation and examples and will release today 🚀

@MasGaNo
Copy link
Author

MasGaNo commented Apr 21, 2019

Hey @piotrwitek

Hehe, you reused finally chaining-handle approach 🙂

Ok, I'm waiting for your release, and I will try it on my-side.

I'm still more in favor of the objectMapCreator approach, but until TypeScript implement the missing feature, it could be definitely a good approach, for the Typing safety.

// NEW FEATURE: extend internal typesafe-actions RootAction type 
// so you never have to use generic types in your application 😮 
// it's as simple as below one-liner
declare module 'typesafe-actions' {
  export type RootAction = ActionType<typeof actions>;
}

Really nice 👍

I never consider this approach!

Will make some try with that 😄

@piotrwitek
Copy link
Owner

piotrwitek commented Apr 21, 2019

Hey @MasGaNo, I think you missed a very important point, I made an implementation that is actually covering both approaches :)

There is the second optional argument in createReducer to provide objectMapCreator, take a look again this time the simple implementation, everything is completely typesafe ⚔️🛡, and you can chain it even further with additional handlers. It's basically an extension mechanism.

const rootReducer = createReducer(initialState, {
 ADD: (state, action) => state + action.payload,
 INCREMENT: (state, _) => state + 1,
})

What do you think?

I worked across all typescript issues and it's even working with getType helper
You can take a look at all the test cases here for more examples: https://github.com/piotrwitek/typesafe-actions/blob/c319c3a7b43671bcdd38215ad13139aabc5afab9/src/create-reducer.spec.ts

@MasGaNo
Copy link
Author

MasGaNo commented Apr 21, 2019

Hey @MasGaNo, I think you missed a very important point, I made an implementation that is actually covering both approaches :)

Yes right 😄

Very exciting to be able to test it!

Unfortunately, I cannot do it now, need to wait tomorrow 😅

I really like the Array approach to solve the problem of "multi-case" from the "switch... case" approach.

Currently, I store the reducerHandler into a variable and I pass it, but here, it's really a nice approach 👍

And you are still opened to let developer modify the generic parameters TState and TAllActions.

So yeah, definitely, looks very good! 👍

I will definitely provide you my feedback tomorrow 👍

piotrwitek added a commit that referenced this issue Apr 21, 2019
<!-- Thank you for your contribution! 👍 -->
<!-- Please makes sure that these checkboxes are checked before submitting your PR, thank you! -->

## Description
Added createReducer - a new API to easily create typesafe reducers

Example refactoring of regular reducer to createReducer: cef1a51

## Related issues:
- Resolved #106 
- Resolved #123

## Checklist

* [x] I have read [CONTRIBUTING.md](../CONTRIBUTING.md)
* [x] I have linked all related issues above
* [x] I have rebased my branch

For bugfixes:
* [ ] I have added at least one unit test to confirm the bug have been fixed

For new features:
* [x] I have updated API docs and tutorial (if applicable)
* [x] I have added short examples to demonstrate new feature usage
* [x] I have added type unit tests with `dts-jest`
* [x] I have added runtime unit tests with `dts-jest`
* [x] I have added usage example in codesandbox project
@issuehunt-oss
Copy link

issuehunt-oss bot commented Oct 30, 2019

@piotrwitek has rewarded $35.00 to @piotrwitek. See it on IssueHunt

  • 💰 Total deposit: $50.00
  • 🎉 Repository reward(20%): $10.00
  • 🔧 Service fee(10%): $5.00

@issuehunt-oss issuehunt-oss bot added the 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt label Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants