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

RFC: Simplify middleware signature #1744

Closed
gaearon opened this Issue May 18, 2016 · 7 comments

Comments

8 participants
@gaearon
Collaborator

gaearon commented May 18, 2016

This was proposed numerous times before (#784, happypoulp/redux-tutorial#48), and I still hear about the Redux “obnoxious” middleware signature. I think the complaint comes from the fact that most middleware never uses store argument so it’s far from being clear why it’s required.

I’m not a big fan of changing things, but we have barely made any breaking changes since the release, and since we have an overhaul for store enhancers in #1702, I realized that if I was working on Redux 1.0 today, I’d keep the middleware signature simpler. I’m happy to release 4.0 alpha and keep it as next version for several months to let middleware authors catch up.

The signature I’m proposing is:

const logger = (next) => (action) => {
  // ...
}

Note that most middleware only care about the next argument so this is why it should be the first rather than store. You can then get store if you need to read the state or dispatch from the beginning of the chain—both are advanced use cases:

const thunk = (next, store) => (action) => {
  // ...
}

As a migration strategy, I suggest a compat library:

import { createStore, applyMiddleware } from 'redux'
import { wrapCompatMiddleware } from 'redux-compat'

const store = createStore(
  reducer,
  applyMiddleware(
    thunk, // new-style middleware
    wrapCompatMiddleware(logger), // middleware that hasn't been updated yet
  )
)

This will let consumers jump into 4.x right away but since they’d have to add a tiny compat package, that would put some pressure onto middleware authors to update their middleware for 4.x.

Finally, we can avoid doing this at all. However I don’t see another round of breaking changes like 4.x, so this is the only opportunity where I’m ready to do it. I’ve been telling people for months that “it’s just currying” but I can’t give a compelling reason for it other than “we just happened to choose this style”. And I do think this style is confusing to beginners, just like the original style of applying enhancers was. I think we improved it in 3.1.0, and I think we can improve this as well.

What do you think?

@gaearon gaearon added the discussion label May 18, 2016

@gaearon gaearon added this to the 4.0 milestone May 18, 2016

@brycehanscomb

This comment has been minimized.

Show comment
Hide comment
@brycehanscomb

brycehanscomb May 18, 2016

What would be the considerations for the long-term management of redux-compat?

Since the nature of compat libs like this is to help across versions, how would you manage this if wrapCompatMiddleware needs to behave differently for some later inter-version compatibility?

Maybe we have redux-compat-compat? :-P

brycehanscomb commented May 18, 2016

What would be the considerations for the long-term management of redux-compat?

Since the nature of compat libs like this is to help across versions, how would you manage this if wrapCompatMiddleware needs to behave differently for some later inter-version compatibility?

Maybe we have redux-compat-compat? :-P

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin May 18, 2016

I'm unconvinced. This would inconvenience "ordinary" users for a minor gain for middleware authors.

There are far more users than middleware authors, and this only comes up when you first create a new middleware.

glenjamin commented May 18, 2016

I'm unconvinced. This would inconvenience "ordinary" users for a minor gain for middleware authors.

There are far more users than middleware authors, and this only comes up when you first create a new middleware.

@nickpresta

This comment has been minimized.

Show comment
Hide comment
@nickpresta

nickpresta May 18, 2016

I know this is only a single sample but we have 5 custom middlewares of various complexity (some for analytics, some for integrating with third party plugins that hijack window, etc) and we use the store argument in 4 of them.

I like that the signature is explicit and is really only "ugly" when using arrows, IMO.

nickpresta commented May 18, 2016

I know this is only a single sample but we have 5 custom middlewares of various complexity (some for analytics, some for integrating with third party plugins that hijack window, etc) and we use the store argument in 4 of them.

I like that the signature is explicit and is really only "ugly" when using arrows, IMO.

@natenorberg

This comment has been minimized.

Show comment
Hide comment
@natenorberg

natenorberg May 18, 2016

Is the only reason for the change to make it less confusing to those that are new to redux?
Personally, I think it's more confusing to have an API change that makes tutorials inconsistent.

Right now you can just google currying or memorize that middleware starts with const middleware = store => next => action => {...}. Sorting out different versions of tutorials is much more confusing.

natenorberg commented May 18, 2016

Is the only reason for the change to make it less confusing to those that are new to redux?
Personally, I think it's more confusing to have an API change that makes tutorials inconsistent.

Right now you can just google currying or memorize that middleware starts with const middleware = store => next => action => {...}. Sorting out different versions of tutorials is much more confusing.

@azu azu referenced this issue May 18, 2016

Closed

Redux #58

5 of 8 tasks complete
@acstll

This comment has been minimized.

Show comment
Hide comment
@acstll

acstll May 18, 2016

I have played with this before (#651) too so I'm gonna share my 2 cents.

To avoid changing the signature but also allowing beginners (or people with a "personal dislike" for currying ;-)) to write simple middleware functions, you could use lodash curry (or something similar) to wrap every middleware function. (I haven't tested this now, but I remember doing it some time ago).

Sort of like:

import curry from 'lodash/curry'

// .. then

chain = middlewares.map(middleware => curry(middleware)(middlewareAPI))

(you could also check function arity (length) to wrap it or not, but I think wrapping anyways won't hurt)

Then users could keep authoring middleware with the current signature or just do

function myMiddleware (store, next, action) {
  // ..
}

Note this is practically the same as just wrapping it yourself when authoring, like:

import curry from 'lodash/curry'

function myMiddleware (store, next, action) {
  // ..
}

export default curry(myMiddleware)

acstll commented May 18, 2016

I have played with this before (#651) too so I'm gonna share my 2 cents.

To avoid changing the signature but also allowing beginners (or people with a "personal dislike" for currying ;-)) to write simple middleware functions, you could use lodash curry (or something similar) to wrap every middleware function. (I haven't tested this now, but I remember doing it some time ago).

Sort of like:

import curry from 'lodash/curry'

// .. then

chain = middlewares.map(middleware => curry(middleware)(middlewareAPI))

(you could also check function arity (length) to wrap it or not, but I think wrapping anyways won't hurt)

Then users could keep authoring middleware with the current signature or just do

function myMiddleware (store, next, action) {
  // ..
}

Note this is practically the same as just wrapping it yourself when authoring, like:

import curry from 'lodash/curry'

function myMiddleware (store, next, action) {
  // ..
}

export default curry(myMiddleware)
@Silverwolf90

This comment has been minimized.

Show comment
Hide comment
@Silverwolf90

Silverwolf90 May 18, 2016

On a pragmatic level, I think this change is not at all worth the requirement of migration.

On a more abstract level, the middleware signature feels consistent in the "spirit" of the library. It's just a curried function; there's nothing special or "obnoxious" about it. It may be confusing to beginners with no exposure to FP-style JS, but that's no different than people trying to learn all the popular JS libraries at once. Once thing at a time. It's up to the docs to point people in the right direction if they don't have the necessary information.

Silverwolf90 commented May 18, 2016

On a pragmatic level, I think this change is not at all worth the requirement of migration.

On a more abstract level, the middleware signature feels consistent in the "spirit" of the library. It's just a curried function; there's nothing special or "obnoxious" about it. It may be confusing to beginners with no exposure to FP-style JS, but that's no different than people trying to learn all the popular JS libraries at once. Once thing at a time. It's up to the docs to point people in the right direction if they don't have the necessary information.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 19, 2016

Collaborator

OK, seems like this is controversial so not worth it.

Collaborator

gaearon commented May 19, 2016

OK, seems like this is controversial so not worth it.

@gaearon gaearon closed this May 19, 2016

@timdorr timdorr removed this from the 4.0 milestone Nov 3, 2017

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