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

Question about middlewareAPI #2097

Closed
ccniuj opened this issue Nov 12, 2016 · 5 comments
Closed

Question about middlewareAPI #2097

ccniuj opened this issue Nov 12, 2016 · 5 comments

Comments

@ccniuj
Copy link

ccniuj commented Nov 12, 2016

In applyMiddleware.js:

export default function applyMiddleware(...middlewares) {
  return (createStore) => (reducer, preloadedState, enhancer) => {
    var store = createStore(reducer, preloadedState, enhancer)
    var dispatch = store.dispatch
    var chain = []

    var middlewareAPI = {
      getState: store.getState,
      dispatch: (action) => dispatch(action)
    }
    chain = middlewares.map(middleware => middleware(middlewareAPI))
    dispatch = compose(...chain)(store.dispatch)

    return {
      ...store,
      dispatch
    }
  }
}

I think we can simply pass dispatch instead of wrapping it in a lambda:

export default function applyMiddleware(...middlewares) {
  return (createStore) => (reducer, preloadedState, enhancer) => {
    var store = createStore(reducer, preloadedState, enhancer)
    var dispatch = store.dispatch
    var chain = []

    var middlewareAPI = {
      getState: store.getState,
      dispatch
    }
    chain = middlewares.map(middleware => middleware(middlewareAPI))
    dispatch = compose(...chain)(store.dispatch)

    return {
      ...store,
      dispatch
    }
  }
}

The code above passes all the tests except for this one:

applyMiddleware › passes recursive dispatches through the middleware chain
expect(received).toEqual(expected)
Expected value to equal:
  2
Received:
  1

So here's my question: Is there any specific concern about the original version of middlewareAPI? Or is it okay to improve it?

@jimbolla
Copy link
Contributor

jimbolla commented Nov 12, 2016

Here's the commit where that was added: af474ba. Unsure why. Is the current implementation causing you problems?

@ccniuj
Copy link
Author

ccniuj commented Nov 12, 2016

@jimbolla
Thanks for your reply. This is not really causing any problem but just making me wonder is there any insight behind the current implementation.

@sunkai612
Copy link

@davidjuin0519
Not sure if "to only compose dispatch method once" is the reason for that lambda.

@markerikson
Copy link
Contributor

This has been asked in previous issues. I don't have a link to the specific official answer at the moment, but I believe it's to set up a closure referring to the original dispatch function.

@ccniuj
Copy link
Author

ccniuj commented Nov 13, 2016

@markerikson
Thanks for your reply. I think I found the discussion #1592 which reveals that the purpose of using a closure is to bind dispatch dynamically. 😀

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