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

Reducer parameter order is reversed from how traditional reducers work, why? Should it be swapped to prevent confusion and make it easier to convert from Redux? #179

Closed
jeffbski opened this issue Jul 19, 2016 · 6 comments
Milestone

Comments

@jeffbski
Copy link

Array, Redux, and rxjs reducers all have the convention that the accumulated state is the first parameter.

Why have you instead chosen to have the data/action to be first? That just seems like it is going to cause lots of confusion when people are used to using reducers elsewhere and now in this case it is backwards.

I know that I would be frustrated having to use one convention with arrays and then the opposite when working with choo.

Also since many people might be coming to choo from React + Redux, all of their existing redux reducers will need to be reworked to use them in choo, otherwise the conversion would be much easier.

Would you consider swapping the order to use the common accepted signature (state, data)? or if not then is there a good reason?

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 20, 2016

Eh, it was for no particular reason. I'm not too fussed about argument order to be honest, kinda used to what choo does. But um, yeah sure happy to change it if people feel strongly about this - it'd be a breaking change tho. What do others think?

@joakin
Copy link

joakin commented Jul 20, 2016

The term reducer used by redux and other libraries comes from the function that you apply every step of the way when using reduce, as in [1, 2, 3].reduce(reducer, 0) in JS.

The typical signature for a reducer in most programming languages is

function reducer (accumulator, value, index) { ... }

In the case of actions applied over models, I believe that the array to be reduced is the actions, and the accumulator is the state to which we want to apply the actions. So given an array of actions, it should be possible to calculate the resulting state (by reducing over them using the reducer and the state as the initial accumulator). A little example:

const actions = ['incr', 'incr', 'decr']
const state = {counter: 0}
const counterReducer = (state, action) =>
  state.counter + (action == 'incr' ? 1 : (action == 'decr' ? -1 : 0))

const nextState = actions.reduce(counterReducer, state)
console.log(nextState)

This is where the redux and others reducers params state, action come from, to stay compliant with JS [].reduce.

I don't think it's a very important issue, but it would be a nice touch that things called reducer could be passed to a actions.reduce(reducer, state) with normal JS without having to flip it, but it is nothing critical 😄

@MattMcFarland
Copy link
Contributor

Breaking changes are never fun, but I think this is worth it, especially sooner rather than later, as later down the road we might be it harder to fix and deal with the fallout.

It might be worth doing in a later (but soonish) release, like after a lot of bug fixes maybe?

@yoshuawuyts
Copy link
Member

Yeah, from talking to a bunch of people it seems most people are on board with this change - it would then become part of 4.0, packed together with a bunch of other breaking changes (created a draft of what a future API might look like with choo for an IRC discussion yest: https://gist.github.com/yoshuawuyts/b3b43f12a7160287f3574cd735f18e69).

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jul 21, 2016

Additional note: just wanted to put out there that we care first and foremost about creating a great experience. If there's a need to break the API we're not shy to do it; but we'll always be transparent in what changed, why it changed and what steps are needed to migrate. I think it's the only possible way to keep choo nimble

@yoshuawuyts
Copy link
Member

Closed by #268

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

No branches or pull requests

4 participants