-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
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
Conversation
+1 |
This might be a stupid question, but how would I skip providing createStore(
reducer,
undefined, // no initial state
applyMiddleware(thunk, batchedUpdates, NewRelicErrorLoggingMiddleware)
) |
@tedpennings According to the source, you'd just leave out the createStore(
reducer,
applyMiddleware(thunk, batchedUpdates, NewRelicErrorLoggingMiddleware)
) |
Nice, I like it. |
awesome, thanks @holic -- I like this a lot |
Yes, we can detect you didn't pass the initial state because valid state can never be a function. |
👍 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(). |
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. |
👎 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 const enhancers = compose(
applyMiddleware(thunk, logger),
DevTools.instrument()
)
const store = createStore(
reducer,
initialState,
enhancers
) |
I like that this proposal allows us to drop |
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. |
No clue :D |
It's a parenthesis mess. 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.
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 |
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. |
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
Or ExpressJS style (doesn't feel FP enough though):
Just throwing some thoughts out. |
Yeah, that's definitely true. This isn't a lisp! 😄
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 |
No, there are very specific reasons why the underlying types have to be exactly as they are now. Store enhancers must be |
@timdorr I kinda like your suggestion. I really wanted to part ways with |
I think this is a good idea; however, I 100% agree with @tj here:
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. |
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? |
@gaearon that looks pretty good to me |
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]
) |
@gaearon Hmm didn't think about omitting |
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]
}) |
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. |
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. |
@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 |
We can implement your "process" proposal as middleware, just like sagas today. Can't we? |
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
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 |
I don't yet understand but I feel like I'm close. Can you create a 10 line repo showing the problem? |
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; |
Demo 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 |
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 |
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. |
Ok, will create an issue. Thanks! |
Add first-class support for store enhancers to createStore() API
Out in 3.1.0. |
(Updated to @timdorr’s suggestion in #1294 (comment))
TL;DR
Fixing the Store Enhancer API
It’s often said that creating a store with enhancers like
applyMiddleware()
orDevTools.instrument()
is weird because it just looks outlandish in JavaScript to compose functions in such a savage way: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 tocreateStore()
previously.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?