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 default handler in createReducer #156

Open
xahon opened this issue May 22, 2019 · 14 comments · May be fixed by #264
Open

Add default handler in createReducer #156

xahon opened this issue May 22, 2019 · 14 comments · May be fixed by #264

Comments

@xahon
Copy link

xahon commented May 22, 2019

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)

Submitted pull Requests


Become a backer now!

Or submit a pull request to get the deposits!

Tips

@piotrwitek
Copy link
Owner

piotrwitek commented May 22, 2019

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:

  • Clear store on user sign out
const rootReducer = combineReducers({
    session,
    organization,
    organizationAccount,
});

export type RootState = StateType<typeof rootReducer>;

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

@piotrwitek piotrwitek changed the title Default handler in createReducer? Add default handler in createReducer May 22, 2019
@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jun 18, 2019
@issuehunt-oss
Copy link

issuehunt-oss bot commented Jun 18, 2019

@issuehunt has funded $80.00 to this issue.


@vladimiry
Copy link

vladimiry commented Jun 26, 2019

Does the following scenario make sense?

  • If the reducers defined for all the actions then adding default reducer should lead to the TypeScript compilation error.
  • If the reducers for some actions are not defined and there is also no default reducer added then TypeScript compilation error should occur.

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

@piotrwitek
Copy link
Owner

@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
Copy link

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
Copy link
Owner

piotrwitek commented Jun 26, 2019

@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
Copy link

McTano commented Jul 18, 2019

@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
Copy link
Owner

@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
Copy link

McTano commented Jul 18, 2019

@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
Copy link

@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
Copy link

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
Copy link

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

@chrispaynter
Copy link

chrispaynter commented Apr 30, 2020

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
Copy link

bunysae commented Oct 21, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants