store.dispatch was already assigned to the variable dispatch

[toddw]

No description provided.

[markerikson]

Pretty sure this is as intended, to ensure that the correct version gets passed down. Thanks, but going to leave this as-is.

[gaearon]

Why isn't CI failing if this is wrong?

[markerikson]

Still getting used to this whole "maintainer" and "CI" thing. The PR was only open for a few minutes - did I close it before Travis had a chance to run things? For that matter, do we have tests covering that aspect of behavior?

[toddw]

I ran the tests locally and they passed. I don't think this change is wrong… it's just a refactor. I figured @markerikson was suggesting that it was a style thing. I figured using store.dispatch instead of the dispatch variable was just to make it very clear which dispatch method it is using. Personally that's not my style but a reasonable decision if that's really why it was that way.

[markerikson]

I know there's been previous PRs that tried to make tweaks inside applyMiddleware without fully understanding what the intent is, and it's definitely a slightly fiddly bit of code. Also, per my comment in #2145 (comment) , my default approach at the moment is to say "no" to code changes unless it's obvious there's a very good reason to make the change. Also not keen on making changes just for code style, overall.

Glancing at the diff again, I think this would work okay because we're not dealing with a closure or anything, but I can also see value in keeping the reference explicit.

[toddw]

In my opinion it is way more clear to have dispatch = compose(...chain)(dispatch). It makes it clear what is going on: we are re-assigning the dispatch function by letting compose do it's thing to it. Before this change, at first it looked like dispatch was being re-assigned to something entirely new using store.dispatch. That struck me as odd until I realized it was just modifying the existing variable with the compose function. That's why I created a PR to make it easier to understand. There is also the weak argument that having to lookup store.dispatch is a waste of cycles since it was already assigned to a variable. The performance penalty of the current code is probably not worth considering but in this PR the code would be easier to read and very slightly more performant.

[toddw]

I've seen you quote your approach several times… I don't have a problem with that but you might want to update the CONTRIBUTING document. If I knew the default attitude was to reject code I wouldn't have taken the time to submit a PR.

[markerikson]

Sorry, I'm not trying to discourage PRs. I'm just hesitant to make changes to code that we know works correctly, especially in areas that have churned in the past, and where I have seen erroneous PRs before. Dan's also made the comment that "Redux is basically done", and although there's definitely a bunch of stuff tagged with a 4.0 milestone that would potentially be useful to have, it's not like there's active development going on.

I am admittedly also new to this whole maintainer thing, so I'm not going to claim I exactly know what I'm doing :)

I'm curious if reopening this will actually kick off a CI run. Let's find out.

[markerikson]

That's a "yes", apparently.

[markerikson]

Well, those do pass. Are there any other tests we can add to capture the correct behavior in here?

[toddw]

I don't think this changes the behavior, it just makes it more clear what is going on so people have a easier time understanding it.

[markerikson]

Right, but I'm asking if we can further guarantee that we have the existing behavior captured. Like, I dunno, maybe a test that ensures that whatever is passed in as store.dispatch really does wind up on the end of the pipeline?

A lot of my thoughts on modifying this file are due to having seen previous PRs that thought the use of the closure for the middlewareAPI.dispatch was a mistake, and tried to remove it. I just really want to make sure that we have as much of the intended behavior captured in test form as possible, and if there's anything we don't cover now, if we can get that taken care of.

[JoeCortopassi]

@markerikson if redux is this fragile, can we get some issues opened for tests that need to be written? Would be happy to write some

[markerikson]

I'm not sure if "fragile" is the right word - it's more that Dan, Andrew, and others have made specific tweaks over time (like the dispatch closure example), and other people have skimmed through the code and filed PRs without going through the history to see why those changes were made.

But yeah, more tests would be a good thing. I personally am a lot more familiar with using Redux than I am with the exact history of specific lines of code, so while I know that there have been specific reasons for certain changes in the past, I'm not familiar with what all those changes are. That also means I don't have specific suggestions for new tests off the top of my head.

[gaearon]

Let me start by acknowledging I believe we all have good intentions.
I’m sorry if you feel frustrated by this discussion but please see where we’re coming from.

This is a tiny library that received a lot of attention.
More tests would be welcome (if they are testing the right thing).
We don’t change it often, and tests have barely changed since its initial release when it was just an experiment.

This means all changes need to be reviewed very carefully because we have millions of users (well, not millions of users, but definitely of downloads), and changing something because it “looks better” can easily backfire.

Take #1592 for example. It looked like an innocent change but actually caused a crash in apps: #1644. We had to revert it in #1647 and add a corresponding test.

In this case I haven’t reviewed code in detail and can’t say for sure if it’s safe or not yet. I also know that many maintainers are busy with other projects, and nobody wants to merge a change that could break thousands of apps just because the code looks nicer. I don’t want us to become risk-averse to the point of being silly either but perhaps contributing tests for more edge cases would be a better starting point.

I hope you can appreciate why this isn’t so simple.

[gaearon]

In particular this change is tricky because it exists on the point where we interface with third-party code.

Just like #1592, it seems safe with reasonable assumptions we can make about how middlewares work, but those assumptions may be wrong.

It is not obvious to me that this change is 100% safe. For example what if there is a weird enhancer + middleware combination that reassigns the previously returned store.dispatch post factum inside the middleware() call? Then this would be a breaking change.

One would think this is a silly scenario but one thing I learned maintaining popular libraries is that if some weird edge case can possibly occur, it already exists in somebody’s code.

Is it worth supporting this edge case? Probably not. Should we break this person with a patch version for no other reason than a nicer-looking implementation? I think not either.

I’m happy to take this into next branch though. Would you like to resubmit a PR against it?

[markerikson]

In this case I haven’t reviewed code in detail and can’t say for sure if it’s safe or not yet. I also know that many maintainers are busy with other projects, and nobody wants to merge a change that could break thousands of apps just because the code looks nicer. I don’t want us to become risk-averse to the point of being silly either but perhaps contributing tests for more edge cases would be a better starting point.

Yeah, that's what I was trying to get at. I'd rather err on the side of not accidentally breaking things because I didn't fully understand the exact implications of what was going on.

If a submitted PR fixes an obvious bug or is clearly an improvement I'm absolutely interested, but small changes that are more style-oriented need more justification for going in.

And yeah, #1592 looks like the previous PR that I was thinking of.

[devinivy]

I personally like this change and think you should feel free to accept it as a patch, but you also reserve the right to reject it. It's truly a matter of taste. @gaearon stated the particular case that this causes breakage in users' apps, and it's a real, code-able case. But I think it clearly falls outside the intended usage of redux (especially given the very existence of middleware/enhancers), and redux absolutely should be allowed to accept this change. If the redux collaborators are comfortable supporting an even higher level of stability that precludes a tiny change like this, that's totally cool too! That is an incredibly high level of stability. I just hope redux collaborators don't feel like they're always walking on egg-shells, in fear of enraging users. You all are wonderful and we appreciate your work 👍

[gaearon]

Thanks! I do think bringing this into the next major would be good, especially given that we plan to clean up a bunch of other middleware-related things.

[zazapeta]

Totally agree with @devinivy !
You are doing a very good job here.
And, you can not and you should not try to fix dumbness of your users.

Haters gonna hate.

Cheers 🍻 !

[gaearon]

Let’s not denigrate our users by calling them names. 😉

Even if somebody uses the library in a weird way it doesn’t mean there’s a good reason to break them if we’re not solving an issue for somebody else.

Anyway, this thread is going on a tangent. I’m happy to revisit this if the PR is resubmitted against the next branch but let’s refrain from a generic conversation here.

[Mamoru1234]

I am really interested in this PR, and spend some time on analyzing applyMiddleware function.
Here is result:

return (createStore) => (reducer, preloadedState, enhancer) => {
    // here we have created store
    var store = createStore(reducer, preloadedState, enhancer)
    // remember link to store dispatch
    var dispatch = /*store.dispatch*/null // disable unwrapped dispatching, according to comment in spec
    var chain = []

    var middlewareAPI = {
      getState: store.getState,
      // function late binding(i am not sure that this is correct definition) in JS looks 
      // really fine, works because we assign to dispatch value result of middlewares 
      //composition
      dispatch: (action) => dispatch(action)
    }
    // here is first step middleware creation ---- with modification of store.dispatch?
    // in theory no, this step is designed to provide middlewareAPI to middlewares closure
    // practically i don't know how to achieve this side effect, because store is encapsulated.
    chain = middlewares.map(middleware => middleware(middlewareAPI))
    // here is where 2 step of middleware creation where "next" is formed.
    // result of 2 step is new dispatch function, also in theory without modification
    // of existing store.dispatch(also don't know how to achieve)
    const dispatchCreator = compose(...chain)
    // store.dispatch will be "next" in middleware which is last in chain(expected behavior)
    dispatch = dispatchCreator(store.dispatch)

    return {
      ...store,
      dispatch
    }
  }

Conclusion: I think that store.dispatch is on correct place according to comment in applyMiddleware.spec. And in further release dispatch will be initialized as a null(as in my code).

PS. Functional style code with mutable objects is hell :)

@gaearon @markerikson What do you think about my comments?

[jimbolla]

Based on the discussion above, I took a shot at refactoring+commenting applyMiddleware to more clearly express intent.

export default function applyMiddleware(...middlewares) {
  function enhanceDispatch(store) {
    // `enhancedDispatch` is initialized as store.dispatch in case a middleware dispatches an
    // action during its initialization. The true value of `enhancedDispatch` is not set until
    // after all middleware have initialized. Any actions dispatched during a middleware's
    // initialization will not be intercetpable by the middleware chain.
    let enhancedDispatch = store.dispatch

    const middlewareAPI = {
      getState: store.getState,
      dispatch: function dispatchProxy(action) {
        // Because `enhancedDispatch` is mutable, it needs to be wrapped in a proxy function so
        // that actions dispatched in middleware are interceptable by the entire middleware chain.
        // This allows the middleware and enhancedDispatch to be mutually recursive.
        return enhancedDispatch(action)
      }
    }
    const initializedMiddlewareChain = middlewares.map(middleware => middleware(middlewareAPI))
    enhancedDispatch = compose(...initializedMiddlewareChain)(store.dispatch)
    return enhancedDispatch
  }

  return (createStore) => (reducer, preloadedState, enhancer) => {
    const store = createStore(reducer, preloadedState, enhancer)
    return {
      ...store,
      dispatch: enhanceDispatch(store)
    }
  }
}

[markerikson]

Whoa. Like, comments and stuff. Are we actually allowed to have those in the codebase? :)

Definitely liking where that's headed. Terse code has its place, but I'm all for making things easier to read and understand in context. I'll come back to look at this later, but that certainly looks like a change we'd want to make.

[Mamoru1234]

Hm, I am a bit confused cause it's not clear what is expected behavior?
According to https://github.com/reactjs/redux/blob/master/test/applyMiddleware.spec.js#L99 in a further release, it should be forbidden, and personally, I think that it's unexpected behavior.
@jimbolla refactoring makes sense for me, besides of redundant internal function creation(it can be avoided by passing middlewares as a parameter). Also, his refactoring still allows you to use dispatch during step 1 of middleware creation(as I say before I think it's unexpected)
What do you think?

[jimbolla]

I was preserving existing behavior (otherwise it's not just refactoring). If the intent is to forbid early dispatches in a later release, then declaration of enhancedDispatch changes to:

let enhancedDispatch = () => { throw new Error('Early dispatch is forbidden.') }

and we could do an interim implementation as a warning:

let enhancedDispatch = action => { 
  log.warn('Early dispatch is forbidden. This will throw an error in a future version of Redux.')
  store.dispatch(action)
}

[jimbolla]

Actually, @timdorr has already made the change in the next branch where the initial value of dispatch is to throw an error. Since that's the direction applyMiddleware is going, this PR would be incompatible.