Skip to content

Remove unneeded chain declaration #2928

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

Merged
merged 1 commit into from
Apr 12, 2018
Merged

Conversation

BenBrostoff
Copy link

Hello - I've been reviewing the Redux source and was trying to figure out what the purpose of assigningchain to an empty array was in applyMiddleware. There doesn't seem to be a scenario where chain is not immediately updated to middlewares.map(middleware => middleware(middlewareAPI)). Please let me know if I'm missing something here and thanks for a great library.

@markerikson
Copy link
Contributor

Hmm. I appreciate the PR, and I'm inclined to agree, but I'm also very hesitant to merge any changes to this file at this point. We've had numerous PRs over the last few years trying to "tweak" or "fix" things (usually related to the declaration of dispatch in here), and they've almost all been wrong. The current code is there, it works, and I don't see a particularly pressing need to change it further.

I'll ping @timdorr and @jimbolla just to get their thoughts.

@timdorr
Copy link
Member

timdorr commented Apr 12, 2018

For reference, here's where the code was originally introduced: 00de31b

Even in that form, I don't see why we're storing the chain beforehand. The middleware API doesn't allow for modification of the chain, so retaining it outside of that closure doesn't seem to have any impact. I'd argue you don't even need the variable; just spread the map directly into compose. 🤷‍♂️

@jimbolla
Copy link
Contributor

LGTM

@timdorr
Copy link
Member

timdorr commented Apr 12, 2018

OK, at least we're all equally culpable.

@timdorr timdorr merged commit ee5c563 into reduxjs:master Apr 12, 2018
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.

4 participants