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

Add default handler in createReducer #156

Open xahon opened 5 years ago

xahon commented 5 years ago

Issuehunt badges

How do I create a default action handler. For example loadingReducer should set loading bool to true only if it receives specific actions, otherwise I want to make it false


IssueHunt Summary ### Backers (Total: $80.00) - [issuehunt issuehunt](https://issuehunt.io/u/issuehunt) ($80.00) ### Submitted pull Requests - [#241 Reducers: add default handler](https://issuehunt.io/r/piotrwitek/typesafe-actions/pull/241) --- #### [Become a backer now!](https://issuehunt.io/r/piotrwitek/typesafe-actions/issues/156) #### [Or submit a pull request to get the deposits!](https://issuehunt.io/r/piotrwitek/typesafe-actions/issues/156) ### Tips - Checkout the [Issuehunt explorer](https://issuehunt.io/r/piotrwitek/typesafe-actions/) to discover more funded issues. - Need some help from other developers? [Add your repositories](https://issuehunt.io/r/new) on IssueHunt to raise funds.
piotrwitek commented 5 years ago

Currently, there is no such feature yet, we would need to extend the API to handle this case.

Proposed API:

const loadingReducer = createReducer(initialState)
    .handleAction([action1, action2, ...], (state, action) => {
        return true;
    })
    .defaultHandler((state, action) => {
        return false;
    })

Recipes:

export type RootState = StateType;

export default createReducer(undefined as RootState) .handleAction(signOut, (state, action) => { return rootReducer(undefined, action); }) .defaultHandler((state, action) => { return rootReducer(state, action); }) };

issuehunt-oss[bot] commented 5 years ago

@issuehunt has funded $80.00 to this issue.


vladimiry commented 5 years ago

Does the following scenario make sense?

So you either define the reducers for all the actions or for some of them + the default one.

piotrwitek commented 5 years ago

@vladimiry 1st - yes 2nd - I don't see any benefits from this, for me most of the reducers I use doesn't implement all of the actions, and implementing default one every time is an unnecessary step

vladimiry commented 5 years ago

I don't see any benefits from this

The benefit is that defining the reducers for all the actions would be enforced by TypeScript. But I understand that would be an opinionated thing as not all the dev teams prefer this way, probably the minority.

piotrwitek commented 5 years ago

@vladimiry I agree that might be useful in some cases but it seems too opinionated for a default.

To get enforced exhaustive checking I would propose to create another issue as I don't want to expand the scope of this feature. Proposed solution could be a new API like createExhaustiveReducer.

McTano commented 5 years ago

@piotrwitek Personally, I think a compilation error for adding a default reducer when all cases are covered is too opinionated for a default behaviour, as it's sometimes reasonable to handle impossible cases to protect against gaps in type-coverage or identify them in development.

piotrwitek commented 5 years ago

@McTano I understand your point although you could very easily fill the gaps by extending RootAction type and also this library is heavily opinionated about type-safety as a number one goal.

Secondly, the reason for a compilation error is that action in default case would have a type of remaining unhandled actions (all actions - handled actions). So in your case, it would be never.

McTano commented 5 years ago

@piotrwitek

I agree that the correct type for the action in the default case should be the union of the remaining unhandled actions, and in some cases that would be never.

But, and correct me if I'm wrong, I don't think that would cause a compilation error on its own.

In the case that the defaultHandler takes a function of type (state: State, action: never) => State, that should compile, because the never type is only in the argument position.

Then the natural way to do a visual check for exhaustiveness is to look at the inferred type of the action argument to the handler function. If it's never, you've covered all the cases.

EDIT: I see now that chaining .handleAction() is a compilation error when there are no action types left, so taking the same approach with a default handler would suggest that it should also be a compilation error. I still think it might make sense to allow the default handler when the action-type union is exhausted.

timmarinin commented 4 years ago

@piotrwitek what would it take now to implement the defaultHandler in the way you've described? Ironically, I want to achieve the exact example you've provided.

chrispaynter commented 4 years ago

I've gotten around this recently (at least the sign out problem) by implementing the root reducer pattern that Dan Abramov suggested here.

I'm not sure if I'm missing something, but the following seems to work for me:

import { combineReducers } from 'redux';
import auth from '../modules/auth/reducer';
import feed from '../modules/feed/reducer';
import post from '../modules/post/reducer';
import profile from '../modules/profile/reducer';
import { StateType, Reducer, RootAction } from 'typesafe-actions';
import { signOut } from 'modules/auth/actions';

const appReducer = combineReducers({
  auth,
  feed,
  post,
  profile
});

type RootState = StateType<typeof appReducer>;

const clearOnSignOutReducer: Reducer<RootState, RootAction> = (
  state,
  action
) => {
  if (action.type === signOut().type) {
    state = undefined;
  }
  return appReducer(state, action);
};

export default clearOnSignOutReducer;

The clearOnSignOutReducer could handle any number of actions I guess. But I suppose, to the questions point, you'd have to hoist the particular default action up to the top level, rather than have it handled by the local reducer.

timmarinin commented 4 years ago

@chrispaynter, unless I'm not mistaken, this requires all your reducers to accept undefined as a possible state.

chrispaynter commented 4 years ago

Ah right yes, got you @marinintim.

One way I've gotten around this in the past is to export the default states of all reducers.

Bit more effort (and you have to remember to add new reducers), but did the job (a few years ago!). After a while, new reducers don't get added so much anyway.

Could get you out of the woods at least until something like the above is added to the library.


const rootReducer = (history: History) => {
  const appReducer = combineReducers({
    auth,
    artist,
    hostApplications,
    location,
    user,
    gigs,
    gigOffers,
    host,
    form,
    payments,
    eventCheckout,
    dirty,
    layout,
    products,
    templates,
    productAllocations,
    router: connectRouter(history)
  });

  type RootState = StateType<typeof appReducer>;

  const clearOnSignOutReducer: Reducer<RootState, RootAction> = (
    state,
    action
  ) => {
    if (action.type === signOutSuccess().type) {
      // Reset state if the user has signed out
      state = {
        auth: authDefaultState,
        artist: artistDefaultState,
        hostApplications: hostApplicationsDefaultState,
        location: locationDefaultState,
        user: userDefaultState,
        gigs: gigDefaultState,
        gigOffers: gigOffersDefaultState,
        host: hostDefaultState,
        router: state!.router,
        payments: paymentsDefaultState,
        eventCheckout: eventCheckoutDefaultState,
        dirty: dirtyDefaultState,
        layout: layoutCheckoutDefaultState,
        products: productsCheckoutDefaultState,
        templates: templatesDefaultState,
        productAllocations: productAllocationsDefaultState,
        form: {}
      };
    }
    return appReducer(state, action);
  };

  return clearOnSignOutReducer;
};
export default rootReducer;
const rootReducerWithHistory = rootReducer(history);
export const store = createStore(
  rootReducerWithHistory,
  composeEnhancers(applyMiddleware(thunk, routerMiddleware(history)))
);
bunysae commented 4 years ago

Just started work on it. Will open a PR soon.