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

Revised middleware-declaration function's interface #784

Closed
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@tweinfeld
Copy link

tweinfeld commented Sep 23, 2015

The current middleware declaration function is required to return a function to intercept the first two dependencies used to construct the middleware (curry).

Since the actual middleware construction requires "store" (or its simulated API) and "next" (next middleware's "dispatch") to both be available at the time of the middleware's function declaration, and since they're both available right at the time of "applyMiddleware"'s invocation, there is essentially no need to split the two arguments into different function calls.

This PR includes support for both 2 and 1 (for existing middleware) arguments definition funcs:

const myMiddleWare = store => next => action => next(action)

or

const myMiddleWare = (store, next) => action => next(action)

more friendly towards ES5 newcomers (and Express middleware authors):

var myMiddleWare = function(store, next){

  return function(action){   
    // process action
    ...
    // proceed to next middleware
    next(action)
  }

}

I believe this minor change could promote authors' familiarity and understanding, thus encourage the development of additional middleware to Redux.

Revised middleware-declaration function's interface
Updated middleware tests to include 2 args declaration func (among the 1 arg "thunk" interface).
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Sep 23, 2015

What would the migration process be like? Shall we add warnings for every middleware using the old signature? Shall we support both? Which one would be referenced in the documentation?

@tweinfeld

This comment has been minimized.

Copy link

tweinfeld commented Sep 23, 2015

I can't see how the current signature serves the author, I suggest the following migration steps:

  1. Enable both signatures support, with a fallback to the current sig in case we can't determine the number of arguments by calling .length - as implemented in the PR.
  2. Revise the documentation: Present only the two-args (new) sig, and add a note regarding the migration from the legacy sig.
  3. On the next major release, still support both sigs, assume the new sig (fallback to it) and send out a deprecation warning.

Migration is always a sticky business, but I think this should keep us safe.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Oct 3, 2015

How do we show deprecation message if we don't know the name of the offending middleware?

@tweinfeld

This comment has been minimized.

Copy link

tweinfeld commented Oct 3, 2015

Good Point. We probably can't name it (not in a practical and satisfactory way anyhow), but we can provide a useful hint: The ordinal of the incompatible function.

We can provide warning messages in the line of:

`The ${index}${[["st", "nd", "rd"][index-1] || "th"].join('')} middleware factory function used to invoke 'applyMiddleware' is incompatible with the required signature...`;

Here's one way in which we can slip this message in:

chain = middlewares.map((middleware, index) => middleware.length === 2 ? middleware.bind(undefined, middlewareAPI) : ((middleware)=>{ console.warn(`The...`); return middleware; })(middleware(middlewareAPI)));

What do you think?

@acdlite

This comment has been minimized.

Copy link
Collaborator

acdlite commented Oct 3, 2015

I see zero advantage in changing the signature, other than personal dislike of currying.

@tweinfeld

This comment has been minimized.

Copy link

tweinfeld commented Oct 4, 2015

I personally love FP along with everything if offers, including the currying technique and monads, so I'm "off the hook" here 😄

In our case though, it might not be the ideal choice since (IMHO):

  • It's redundant - Currying typically shines where a function receives its arguments in an unsynchronized fashion (separated by time). That's not the case here since both "store" and "next" are made available at the same time (when the applyMiddleware function is executed)
  • It's cumbersome - Think of the extra fluff ES5 syntax will require just in order to receive arguments (function(..){ return function(..){ Got everything now! } })
  • It's a deterrent - People can't intuitively understand the separation of the first two functions (because it was made out of implementation needs rather than the API's), which might deter them from contributing their middleware to Redux.
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Oct 5, 2015

I think we won't do this unless there are other useful breaking changes to middleware. I can't justify changing the signature and causing dozens of maintainers to release new major versions and then responding to incompatibility issues with previous Redux versions because they didn't enforce peer dependencies or such—and all for their apparent convenience.

If we planned to change middleware significantly I'd get this in, but at the moment we don't. I understand this keeps the friction, but it's almost moot with ES6 which is what people in Redux ecosystem usually write new code in anyway.

Sorry!

@gaearon gaearon closed this Oct 5, 2015

@tweinfeld

This comment has been minimized.

Copy link

tweinfeld commented Oct 5, 2015

My pleasure 👍
I'd be happy to assist should you decide to reconsider.

@markerikson markerikson referenced this pull request Oct 1, 2016

Closed

FAQ updates #1785

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