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

Add first-class support for store enhancers to createStore() API #1294

Merged
merged 5 commits into from
Jan 28, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jan 28, 2016

(Updated to @timdorr’s suggestion in #1294 (comment))

TL;DR

- const createStoreWithMiddleware = applyMiddleware(thunk, logger)(createStore)
- const store = createStoreWithMiddleware(rootReducer, initialState)
+ const store = createStore(
+   rootReducer,
+   initialState,
+   applyMiddleware(thunk, logger)
+ )

Fixing the Store Enhancer API

It’s often said that creating a store with enhancers like applyMiddleware() or DevTools.instrument() is weird because it just looks outlandish in JavaScript to compose functions in such a savage way:

// bad
const createStoreWithMiddleware = applyMiddleware(thunk, logger)(createStore)
const store = createStoreWithMiddleware(reducer, initialState)

// and even worse
const finalCreateStore = compose(
  applyMiddleware(thunk, logger),
  DevTools.instrument()
)(createStore)
const store = finalCreateStore(reducer, initialState)

Here, we propose to integrate the idea of store enhancers into the createStore() API itself. This would keep the concept of enhancers and their API exactly the same but will simplify applying them in real projects. This is also not a breaking change because we didn’t support any additional arguments to createStore() previously.

// not bad
const store = createStore(
  reducer,
  initialState,
  applyMiddleware(thunk, logger)
)

// not too bad either
const store = createStore(
  reducer,
  initialState,
  compose(
    applyMiddleware(thunk, logger),
    DevTools.instrument()
  )
)

It has often been suggested that we do something about this uneasiness but unfortunately I don’t think there was ever any specific proposal that dealt with store enhancers correctly. Most proposals I saw were focused on the middleware which is just one kind of a store enhancer. I think that this proposal should satisfy the needs of the community in terms of a simple API without compromising on the power of store enhancers.

What do you think?

@bradwestfall
Copy link

+1

@tedpennings
Copy link

This might be a stupid question, but how would I skip providing initialState? -- would this work?

createStore(
  reducer,
  undefined, // no initial state
  applyMiddleware(thunk, batchedUpdates, NewRelicErrorLoggingMiddleware)
)

@holic
Copy link

holic commented Jan 28, 2016

@tedpennings According to the source, you'd just leave out the initialState, e.g.

createStore(
  reducer,
  applyMiddleware(thunk, batchedUpdates, NewRelicErrorLoggingMiddleware)
)

@anru
Copy link

anru commented Jan 28, 2016

Nice, I like it.

@tedpennings
Copy link

awesome, thanks @holic -- I like this a lot :bowtie:

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

Yes, we can detect you didn't pass the initial state because valid state can never be a function.

@tj
Copy link

tj commented Jan 28, 2016

👍 I still have to track down some weird mis-config that is making middleware no-op, 99% it's something in my store precedence-ish.. haha. Just my opinion but I find too many positional args looks confusing, they all end up looking like middleware or something. I think variadic is fine if it's all of the same type like applyMiddleware().

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

Just my opinion but I find too many positional args looks confusing, they all end up looking like middleware or something.

Can you throw a few alternatives off the top of your head just to get the ball rolling? I tried a few options with arrays but neither felt satisfactory.

@timdorr
Copy link
Member

timdorr commented Jan 28, 2016

👎 from me.

What's wrong with encouraging good FP concepts? Also, the way that reads is like the reducer and initial state are part of the "stack" of composed store enhancers. It's hard to tell when the configuration ends and the store enhancers begin.

What about a singular enhancer (much like a singular reducer), which can use compose to stack them:

const enhancers = compose(
  applyMiddleware(thunk, logger),
  DevTools.instrument()
)

const store = createStore(
  reducer,
  initialState,
  enhancers
)

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

I like that this proposal allows us to drop compose() from the public API is it is no longer necessary for using the library. Those who want it elsewhere in the code can always use the lodash version.

@timdorr
Copy link
Member

timdorr commented Jan 28, 2016

I like that this proposal allows us to drop compose() from the public API

Yeah, I agree with that. It's weird to provide that when someone might want to use Ramda or a homegrown solution. Redux isn't a FP library, it just leverages FP concepts.

@tj
Copy link

tj commented Jan 28, 2016

Can you throw a few alternatives off the top of your head just to get the ball rolling? I tried a few options with arrays but neither felt satisfactory.

No clue :D

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

What's wrong with encouraging good FP concepts?

It's a parenthesis mess. finalCreateStore and createStoreWithMiddleware are incredible awkward to write and explain. In fact this was one of the reasons I still haven't recorded videos on the middleware—I can't find a way to teach them without writing very weird syntax. See also https://hughfdjackson.com/javascript/does-curry-help/ for a similar perspective.

Those who dig FPisms can always look inside or compose things manually but I've found that usually people turn off their hands at the sight of this blurb and copy-paste it across projects without understanding how it works. Providing a nicer top-level API is the first step towards helping them understand what's happening.

Also, the way that reads is like the reducer and initial state are part of the "stack" of composed store enhancers. It's hard to tell when the configuration ends and the store enhancers begin.

I agree with this criticism. Another problem is it makes the flow unobvious. Actions would hit the enhancers first top-down and then proceed to the reducer which would be weird. If you can offer an alternative way of stacking them that would be backwards compatible, wouldn't be awkward, would make it easy to just use applyMiddleware(), as well as combine it with DevTools, would allow to omit initial state, and wouldn't freak out at arrays as state, I'm keen to hear it!

@tj
Copy link

tj commented Jan 28, 2016

I've found that usually people turn off their hands at the sight of this blurb and copy-paste it across projects without understanding how it works.

This is totally me hahaha. My first impression when I see lots of distinct calls like that is "oh god, this is complicated", even if they're all doing fundamentally similar things.

@AntJanus
Copy link

I'm not a big fan of the variadic function solution. The original solution is good but it does feel clunky. It feels like you should be able to apply middleware at the same time as creating the store. How about an applyMiddleware chain function? Like so:

const store = createStore(reducer, initialState).applyMiddleware(thunk, logger);

Or ExpressJS style (doesn't feel FP enough though):

const store = createStore(reducers, initialState);

store.use(thunk, logger);

//or
store.use(thunk);
store.use(logger);

Just throwing some thoughts out.

@timdorr
Copy link
Member

timdorr commented Jan 28, 2016

It's a parenthesis mess.

Yeah, that's definitely true. This isn't a lisp! 😄

If you can offer an alternative...

I think my suggestion satisfies this. Single reducer function, single enhancer function. Since we commonly export a root reducer in our examples and in the the real world, we can also export our enhancers the same way. You'd end up with this:

import rootReducer from '../reducers'
import rootEnhancer from './enhancers'

const store = createStore(
  rootReducer,
  initialState,
  rootEnhancer
)
export default const enhancers = compose(
  applyMiddleware(thunk, logger),
  DevTools.instrument()
)

Keeps parity with how you lay out reducers, so it's familiar. And it's backwards compatible, since it's just a new arg on createStore

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

@AntJanus

No, there are very specific reasons why the underlying types have to be exactly as they are now. Store enhancers must be createStore => createStore'. When you use a store enhancer like DevTools it actually creates another store internally and gives you a projection of that store. We can only do that if we wrap createStore itself. We can't let you create a store because by this point it's too late. This is why those fluent APIs and .use() calls don't work here.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

@timdorr I kinda like your suggestion. I really wanted to part ways with compose() but maybe this is our fate.

@acdlite
Copy link
Collaborator

acdlite commented Jan 28, 2016

I think this is a good idea; however, I 100% agree with @tj here:

Just my opinion but I find too many positional args looks confusing, they all end up looking like middleware or something. I think variadic is fine if it's all of the same type like applyMiddleware().

I would suggest making third argument an (optional) array of enhancers:

const store = createStore(
   rootReducer,
   initialState,
   [applyMiddleware(thunk, logger), DevTools.instrument(), ...etc]
 )

EDIT: also fine with the third param being a single enhancer, but if the goal is to get rid of compose, then I prefer an array to variadic args.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

So, per @timdorr:

// not bad
const store = createStore(
  reducer,
  initialState,
  applyMiddleware(thunk, logger)
)

// not bad either
const store = createStore(
  reducer,
  initialState,
  compose(
    applyMiddleware(thunk, logger),
    DevTools.instrument()
  )
)

No variadic stuff, probably more static analysis friendly too. Thoughts?

@tj
Copy link

tj commented Jan 28, 2016

@gaearon that looks pretty good to me

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

I would suggest making third argument an (optional) array of enhancers:

I wanted to do this but then watch out:

// I want to allow to omit the initial state
const store = createStore(
   rootReducer,
   [applyMiddleware(thunk, logger), DevTools.instrument(), ...etc]
 )

// Haha good luck telling array initial state from array of enhancers
const store = createStore(
   rootReducer,
   [{ lol: true }, { sad: true }],
   [applyMiddleware(thunk, logger), DevTools.instrument(), ...etc]
 )

@acdlite
Copy link
Collaborator

acdlite commented Jan 28, 2016

@gaearon Hmm didn't think about omitting initialState. So... we would type check the second param to see if it's a function? Eh, I'm not a huge fan of that but idk.

@BerkeleyTrue
Copy link

How about a single argument function?

const store = createStore({
  reducer,
  initialState,
  enhancer: applyMiddleware(thunk, logger)
})

And/Or

const store = createStore({
  reducer,
  initialState,
  enhancer: [thunk, logger]
})

@yelouafi
Copy link

How would you make an existing store magically change its state based on what devToolsStore tells it?

With the above solution. The simplest way I see is to have it builtin into the store api (some resetState method), I don't think this is a pb since it'll only serve for developer tools.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

I like the current system because it doesn't require internal methods like setState() which no doubt will be abused. I don't see a big issue with how store enhancers work but I'm happy to consider specific alternatives. We spent some hair pulling figuring out the current API and while I understand things are not perfect, I don't have time to reinvent it from the ground up again. If you have specific proposals I'm happy to consider a joint RFC showing new (specific and verified to run correctly) API for composing "new" store enhancers and how that would solve real problems justifying the churn in the ecosystem that would come out of that.

@yelouafi
Copy link

@gaearon I understand. All what I said was a bit of quick thinking :). Perhaps keeping the actual API and providing the action to subscribers as an alternative way (unless there are issues with that), wiil gives a chance to compare how the 2 models works in real-life

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

We can implement your "process" proposal as middleware, just like sagas today. Can't we?

@yelouafi
Copy link

The issue in my case is other: I used an enhancer like method to implement runSaga which can run sagas outside the middleware env (server side rendering, code splitting ...). I didn't use compose but the result would be similar : patch dispatch function to get the dispatched action. But it has some drawbacks; as in redux-saga/redux-saga#48

saga called with runSaga can dispatch actions to store and all sagas from middleware will see it, but actions dispatched from middleware sagas can't be taken by saga called with runSaga

Since dispatch of inside (passed to the middleware) is not the same as dispatch outside (the final that the enhancer get); the runSaga can only patch the final dispatch returned by applyMiddleware enhancer; So the intermediate dispatch (passed to redux-saga middleware) is not patched and then runSaga can't intercept those actions

I thought if store.subscribe can guarantee the delivery of each action then it wont be necessary to enhance/patch the store. Maybe there other cases where the store enhancer needs just the actual action, so it won't be necessary to complicate the store setup if they could just use subscribe

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

I don't yet understand but I feel like I'm close. Can you create a 10 line repo showing the problem?

@yelouafi
Copy link

Here is a jsbin demo.

There are 2 middlewares sagas and 1 saga outside (with runSaga). the outside saga dispatch a RUNSAGA_ACTION and both middleware sagas take it;

Then the 1st middleware saga dispatch a MIDDLEWARE_SAGA action; but only the 2nd middleware saga can take it; the saga outside don't see it;

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

Demo is empty. Wrong link?

@yelouafi
Copy link

Demi is empty. Wrong link?

didn't understand, pb with the link or with or running the demo; re-tried it and it works for me

http://jsbin.com/gezizu/9/edit?js,console

and gist

https://gist.github.com/yelouafi/baf20b6e7e2685d6f08d

@yelouafi
Copy link

perhaps a lifted reducer like used in devtools will work; Is this the only way a store enhancer can get an action from the store

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

Maybe JSBin is broken on mobile. I'll check when I'm at desktop. Can you please file a separate issue? I'll be happy to discuss there as seems mostly unrelated to this thread.

@yelouafi
Copy link

Ok, will create an issue. Thanks!

gaearon added a commit that referenced this pull request Jan 28, 2016
Add first-class support for store enhancers to createStore() API
@gaearon gaearon merged commit 0814ef3 into master Jan 28, 2016
@gaearon gaearon deleted the enhancers branch January 28, 2016 17:48
@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

Out in 3.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet