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

Revised middleware-declaration function's interface #784

Closed
wants to merge 1 commit into from

Conversation

tweinfeld
Copy link

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.

Updated middleware tests to include 2 args declaration func (among the 1 arg "thunk" interface).
@gaearon
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
Copy link
Author

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
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
Copy link
Author

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
Copy link
Collaborator

acdlite commented Oct 3, 2015

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

@tweinfeld
Copy link
Author

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
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
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

3 participants