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

Update applyMiddleware enhancer to use new createStore signature #1302

Merged
merged 1 commit into from
Jan 28, 2016

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jan 28, 2016

3.1 changed the signature of createStore(). This isn't considered a breaking change because existing enhancers will continue to work; however, going forward, it should be expected that a store enhancer returns a createStore function with the same signature as its input.

See https://twitter.com/acdlite/status/692772885186764802 for context

Still needs tests :)

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

Maybe just ...rest? Not that enhancers ever have to deal with other enhancers.

@acdlite
Copy link
Collaborator Author

acdlite commented Jan 28, 2016

Yeah true. Although let's ask the question: should the enhancer passed to createStore be applied before or after the enhancer in question (applyMiddleware in this case)?

I think this is right (before) but... hmm.

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

We don't ever want to encourage mixing both patterns. We'll transition docs to only use the new pattern, and the fact that the old one works will become an implementation detail.

@acdlite
Copy link
Collaborator Author

acdlite commented Jan 28, 2016

Even if we don't encourage it, I still think it's important as an implementation detail. At the least, it will be important for enhancer authors.

Problem in a nutshell:

const finalCreateStore = DevTools.instrument()(createStore)
const store = finalCreateStore(
  reducer,
  initialState,
  applyMiddleware(thunk, logger)
)

Which gets executed first: the DevTools, or the middleware?

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

In this case, middleware will wrap the previously wrapped-with-DevTools function.
Specifying enhancer means specifying "the last wrapper".
Seems right to me.

(But really, nobody should do it)

gaearon added a commit that referenced this pull request Jan 28, 2016
Update applyMiddleware enhancer to use new createStore signature
@gaearon gaearon merged commit 9ffeda7 into master Jan 28, 2016
@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

I trust that this works but please feel free to add tests. I'll release it as a patch bump now.

@gaearon gaearon deleted the applyMiddleware-new-signature branch January 28, 2016 18:51
@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

Out in 3.1.1.

@acdlite
Copy link
Collaborator Author

acdlite commented Jan 28, 2016

In this case, middleware will wrap the previously wrapped-with-DevTools function.

@gaearon I don't think that's right. applyMiddleware() gets applied first because it's passed all the way through to the base createStore().

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

Oh, right. Lol this is counter-intuitive. But then again, it's not a breaking change since it's a new way of doing things. I don't think people will mess this up in their apps but we'll see. Even if they do mess up, we'll just offer to use the new way.

@acdlite
Copy link
Collaborator Author

acdlite commented Jan 28, 2016

Alright as long as we're aware of it :)

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.

None yet

2 participants