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

An alternative implementation for Middleware #651

Closed
acstll opened this issue Aug 28, 2015 · 15 comments
Closed

An alternative implementation for Middleware #651

acstll opened this issue Aug 28, 2015 · 15 comments

Comments

@acstll
Copy link

acstll commented Aug 28, 2015

Hey there, I've been working on this for the last few days and I think it's ready to share.

The whole and only point of this is to make middleware easier to grasp and understand (whether you’re a newcomer or have some FP experience).

The middleware signature is this:

function (action, next) {
  const { getState, dispatch } = this
  next()
}

I created a repo and prepared a README explaining how it works, please go take a look. In the tests I tried to cover all use cases I've seen around. Let me know if you find any quirks or anything missing.

https://github.com/acstll/chopped-middleware

I can imagine 3 different scenarios if you think this could be a step forward:

A. Redux adopts (adapts) this new implementation in a future major release
B. Redux adopts it as a separate module and remove current implementation from core (also future release)
C. Nothing happens, everything stays as it is now

One downside of A and B would be refactoring the current userland middleware modules, but that shouldn't be too hard.

Any feedback is welcome!

@alkhe
Copy link
Contributor

alkhe commented Aug 28, 2015

This looks somewhat like the middleware fluxette uses.

It basically removes the need for the extra function call, by binding this to the middleware instance. Refactoring would be especially easy.

This is the thunk middleware that uses this system.

@acstll
Copy link
Author

acstll commented Aug 29, 2015

@edge yes it's similar. I did try that too, but thought that removing just one function wasn't worth the change. I wanted to remove both nested functions.

The thunk middleware would look like this:

function thunk (action, next) {
  if (typeof action === 'function') {
    return action(this)
  }
  next()
}

Binding the store methods (dispatch and getState) to the middleware context is just a choice I made. It's possible also to pass it as an argument. It'd look like this:

function (action, store, next) {
  next()
}

I just read (in fluxette's README 😄) the current "bound" implementation could break if people defined middleware using the arrow function, I didn't thought of that before but it could be a problem indeed…

UPDATE:

as an example, thunk would look like this without the binding:

function thunk (action, store, next) {
  if (typeof action === 'function') {
    return action(store)
  }
  next()
}

@gyzerok
Copy link

gyzerok commented Aug 29, 2015

Its implicitness. I'm highly against this way.

@acstll
Copy link
Author

acstll commented Aug 29, 2015

@gyzerok are you against binding the store methods to this or to this whole implementation?

If the former, I prefer explicitness too.

@gyzerok
Copy link

gyzerok commented Aug 29, 2015

@acstll as I've understood binding to this is the whole implementation.

@acstll
Copy link
Author

acstll commented Aug 29, 2015

@gbezyuk oh no, no at all. Let me further explain.

The implementation uses the step.js module to fire the middleware functions, as opposed to nesting and composing callbacks.

Here it is: https://github.com/acstll/chopped-middleware/blob/master/index.js#L23-L26 There's no Array#reduceRight.

This allows everything the current implementation does:

  • getState() before and after the state update
  • Transform the action object
  • Skip dispatching
  • Call dispatch(newAction) and have the new action go through the chain

And the middleware function signature is a lot much simpler:

function foo (action, /* store, */ next) {
  next()
}

instead of

function foo (store) {
  return function (next) {
    return function (action) {
      return next(action)
    }
  }
}

// ES2015
const foo = store => next => action {
  return next(action)
}

which can give you a headache the first time you read it, though the functional approach is beautiful :-)

@gbezyuk
Copy link
Contributor

gbezyuk commented Aug 29, 2015

@acstll, I guess it was not addressed to me XD

@gyzerok
Copy link

gyzerok commented Aug 29, 2015

@acstll IMO it can be a 3rd party Redux tool but it shouldnt be added to the core.

@acdlite
Copy link
Collaborator

acdlite commented Aug 29, 2015

If you don't like the multiple levels of functions, you can use a curry function:

function middleware(store, next, action) {
  return next(action);
}

export default curry(middleware);

@acstll
Copy link
Author

acstll commented Aug 29, 2015

@acdlite currying is a perfect solution for my problem, but it doesn't remove any complexity.

I get the point though. 😄

@alkhe
Copy link
Contributor

alkhe commented Aug 29, 2015

I agree with @acstll on this one. Currying the middleware export introduces a new and unnecessary dependency, at which point most people would defer to writing the functions manually anyways.

Currying in the core just adds bulk and introduces more implicitness.

@acdlite
Copy link
Collaborator

acdlite commented Aug 29, 2015

I'm not suggesting adding to core. Use it for authoring middleware if it makes you feel better.

@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2015

As stated here:

I understand your concerns, maybe you're right. Maybe it breaks some use case we haven't considered. I don't know. But this feedback would have been more useful at the time middleware was designed (it was all publicly discussed: #55). Right now there is a lot of middleware in the ecosystem that uses the current signature, and unless you see a way to change it in backwards-compatible way, I don't think “remove store =>” is a reason compelling enough to break everyone.

@gaearon gaearon closed this as completed Sep 4, 2015
@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2015

In other words: once you get used to currying you no longer see it as a form of complexity. It becomes the same sort of issue as “named args” vs “positional args”—mostly a matter of style. A style change that would break everyone in the ecosystem isn't worth it if it doesn't also bring some new amazing possibilities.

@acstll
Copy link
Author

acstll commented Sep 4, 2015

once you get used to currying you no longer see it as a form of complexity

exactly, you need to get used to it and that takes time. It's a fact that the current implementation is more complex than a single callback, otherwise Middleware wouldn't be in the Advanced section of the documentation 😉

A style change that would break everyone in the ecosystem isn't worth it if it doesn't also bring some new amazing possibilities

I agree with this a 100% 👍

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

No branches or pull requests

6 participants