Reducers should receive whole action

[erin-noe-payne]

First of all, I'm really liking the library - thanks!

However, a small complaint I have is that the reducer functions currently receive only the action payload:

const setName = actionCreator<string>("SET_NAME");
function setNameHandler(state: State, name: string): State {
    return { ...state, name };
}

This is an issue, because FSA's include some data on the root level object that may influence the logic of my reducer - in particular the error flag and meta properties.

Consider this example:

const GetDataResponse = actionCreator<DataResponse | Error>("GET_DATA_RESPONSE")
function dataResponseHandler(state: State, name: DataResponse | Error) {
  // ... do my reducer stuff
}

// Later, I dispatch an action:
GetDataResponse(new Error('There was an issue retrieving your data'), meta: {statusCode: 500, details : 'There was a problem with the server'});
// In my reducer, I want to reference this action metadata, switch on action.error property, or use some utility functions from the flux-standard-action library.

This should be a relatively small change, which would become:

const GetDataResponse = actionCreator<DataResponse | Error>("GET_DATA_RESPONSE")
function dataResponseHandler(state: State, name: Action<DataResponse | Error>) {
  // ... do my reducer stuff
}

I'm happy to fork and submit a PR with the changes if you agree this is the correct approach.

Thanks!

[dphilipson]

Hi @autoric,

This makes a lot of sense to me as a use case, but I worry that it adds clutter in (what has been my experience been) the common case where only the payload is needed. It's also a pretty big breaking change so I want to be sure this is what people would want.

What are your thoughts on making this backwards compatible with one of the following options?

1. Pass the full action as a third argument to the handler
2. Have a separate method for adding handlers that take an action, something like

   reducerWithoutInitialState<DataResponse | Error>()
       .caseWithAction(GetDataResponse, dataResponseHandler);

[erin-noe-payne]

Ultimately, it's your library - so whatever you think is the best approach!

My personal opinion would be to just make a breaking change and update semantic versioning appropriately. The migration would be pretty simple:

Before:

function actionReducer(state, payload) { }

After:

function actionReducer(state, {payload}) { }

I understand and agree with your experience that the most typical use case is just accessing the action payload. However, in the redux docs its clear that reducers receive actions.

My preference is minimal API surface area, and I think destructuring out the payload would be fairly painless. I also think receiving the whole action is the most expected behavior if you are adopting this library after starting with your own reducers - which will certainly be receiving the whole action object.

Just my 2c. =)

[dphilipson]

The case where it adds clutter is where I'm already destructuring the payload argument. Thus instead of

    .case(setUser, (state, { firstName, lastName }) => ... )

I end up with

    .case(setUser, (state, { payload: { firstName, lastName } }) => ... )

which is kinda ugly and possibly unfamiliar to some readers.

I'm tentatively going to go with option 2 from above (a new .caseWithAction() method).

[erin-noe-payne]

Cool. Let me know if you'd like a PR and I'm happy to take a look at it. Otherwise I will happily use the new feature when you have it done!

Thanks

[dphilipson]

.caseWithAction() implemented in v0.4.0.