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

Proof of Concept: Enhancer Overhaul #1702

Open
wants to merge 1 commit into
base: master
from

Conversation

@gaearon
Collaborator

gaearon commented May 9, 2016

Heavily inspired by #1576 but taken further.
(I temporarily removed JSDoc because everything inside moved.)

The public API for regular consumers stays the same. The public API for people who write store enhancers is simplified.

We add a new concept. I tentatively call it “store base” but its name doesn’t really matter yet. I encourage you to avoid bikeshedding on the name for now, and consider the concept instead.

The “store base” thing is exactly what we currently pass to the middleware. It’s a stripped down version of the store that offers no subscriptions. It’s just { dispatch, getState }. In this PR, we consider Redux if it was implemented around this abstraction.

Since a “store base” doesn’t have a subscription mechanism, a function that creates the store base accepts (reducer, initialState, onChange) as arguments. The mechanism of notifying subscribers is implemented at a higher level by the store.

Making onChange an argument is important for keeping the “store base” a useful composition abstraction. For example, a “store base” enhancer may want to wrap onChange to debounce calls to all store subscribers without knowing anything about the subscriber logic.

I might be wrong but I have a feeling that “store base” enhancers can completely replace store enhancers as the main Redux extension mechanism. Not also do they offer wrapping reducer and initialState, but they also have a number of other benefits that store enhancers don’t:

  • They are easier to write because only two methods need to be returned: getState and dispatch.
  • Unlike store enhancers, “store base” enhancers don’t need to worry about wrapping replaceReducer or copy-pasting our $$observable implementation since both of those things are implemented at the store level which “seals” the composed store base enhancers.
  • “Store base” enhancers that implement batching wouldn’t need to worry about tracking the changes to our change emitter mechanism because they would be able to wrap (to delay or debounce) onChange as they see fit without knowing what’s inside of it.
  • If store “seals off” the enhanced “store base” chain as shown in this PR, enhancers won’t be able to put arbitrary methods on the store. (Arguably some enhancers do that today, but the overall system is less fragile if we just disallow this.)
  • The INIT action goes through all enhancers and middleware because it is emitted outside the “store base” chain in the store itself. The store has the ability to enforce that the initial action is dispatched synchronously, as it can check the current state right after the first dispatch. (#465)
  • This would also leave us with just one API for applying enhancers (the modern one) because the old way would give you a store without a subscribe method.

This doesn’t even quite introduce a new concept, as we already pass a thing with the same API to middleware. We’re just giving it a name (rather than a vague “middleware API”) and using more prominently. Also, it is only relevant to people who write store enhancers.

The only downside I can think of so far is the documentation / tutorial update / ecosystem costs, but luckily we never documented enhancers thoroughly, and we only have a few popular ones. In fact many some enhancers should just work unless they forget to pass all arguments, add custom methods, or specifically handle subscribe (in which case they’d be better off handling onChange instead anyway). And we don’t need to be stuck in an update limbo because we can release a 4.0 alpha that switches enhancer argument to mean a “store base” enhancer, and send some PRs to popular enhancers, and then roll it out when everybody is happy.

Thoughts? I know this is a big change, but we barely had any breaking changes since the release, and if there are no other problems, I feel this moves Redux closer to what I originally tried to pull off with separating dispatching from subscribing when I was first thinking about Redux. I’d be much more comfortable with this store extension mechanism than what we have now, but if I’m wrong please help me understand why.

@acdlite @zalmoxisus @tappleby @tomkis1 @yelouafi @timdorr @lukewestby

@gaearon gaearon added this to the 4.0 milestone May 9, 2016

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

Note that these benefits above are based on my gut feeling. I’ll still need to verify all these problems are actually solved by this abstraction.

Collaborator

gaearon commented May 9, 2016

Note that these benefits above are based on my gut feeling. I’ll still need to verify all these problems are actually solved by this abstraction.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

On first glance this seems really great — I like the separation of dispatch logic and subscription logic. But I don't understand this sentence:

This would also leave us with just one API for applying enhancers (the modern one) because the old way would give you a store without a subscribe method.

Why wouldn't one of today's enhancers continue to work the old way with this new createStore(), which has the same signature as before?

Collaborator

acdlite commented May 9, 2016

On first glance this seems really great — I like the separation of dispatch logic and subscription logic. But I don't understand this sentence:

This would also leave us with just one API for applying enhancers (the modern one) because the old way would give you a store without a subscribe method.

Why wouldn't one of today's enhancers continue to work the old way with this new createStore(), which has the same signature as before?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr May 9, 2016

Member

Interesting. So, at it's core, this can be thought of similarly to rearranging a traditional class inheritance. That is, rather than the enhancer extending the Redux "class", Redux inherits from something that implements a particular interface. (Not sure if that analogy is helpful, but it's what springs to mind at the moment).

Or another way to think of it: Inversion of Control (IoC). So, this isn't unprecedented.

Member

timdorr commented May 9, 2016

Interesting. So, at it's core, this can be thought of similarly to rearranging a traditional class inheritance. That is, rather than the enhancer extending the Redux "class", Redux inherits from something that implements a particular interface. (Not sure if that analogy is helpful, but it's what springs to mind at the moment).

Or another way to think of it: Inversion of Control (IoC). So, this isn't unprecedented.

@azu azu referenced this pull request May 9, 2016

Closed

Redux #58

5 of 8 tasks complete
// reducer returns their initial state. This effectively populates
// the initial state tree.
dispatch({ type: ActionTypes.INIT })
replaceReducer(reducer)
return {
dispatch,

This comment has been minimized.

@acdlite

acdlite May 9, 2016

Collaborator

Should we have the "real" store extend the store base here via spread, so that enhancers can add their own properties besides dispatch and getState?

return {
  ...storeBase,
  dispatch,
  getState,
  subscribe,
  // ... and so on
}

E.g. this would allow the observable interface to be safely be implemented as an enhancer. Not that we'd necessarily want to do that — just an example of how alternative event systems* are possible with this new enhancer architecture.

*not that they weren't possible before, but now they're safer

@acdlite

acdlite May 9, 2016

Collaborator

Should we have the "real" store extend the store base here via spread, so that enhancers can add their own properties besides dispatch and getState?

return {
  ...storeBase,
  dispatch,
  getState,
  subscribe,
  // ... and so on
}

E.g. this would allow the observable interface to be safely be implemented as an enhancer. Not that we'd necessarily want to do that — just an example of how alternative event systems* are possible with this new enhancer architecture.

*not that they weren't possible before, but now they're safer

This comment has been minimized.

@acdlite

acdlite May 9, 2016

Collaborator

In other words, I don't see the benefit of "sealing" the enhancers, and it seems like you lose some extensibility as well.

@acdlite

acdlite May 9, 2016

Collaborator

In other words, I don't see the benefit of "sealing" the enhancers, and it seems like you lose some extensibility as well.

This comment has been minimized.

@gaearon

gaearon May 9, 2016

Collaborator

The problem I tried to solve is that anything other than { getState, dispatch } is hard to compose, and enhancers have no knowledge of the past enhancers. If any enhancer could add a method like subscribe(), it would be a pain to later batch subscriptions in any later enhancer (this is pretty much why https://github.com/tappleby/redux-batched-subscribe is currently using a less performant subscription mechanism which was copy pasted from earlier versions of Redux). If any enhancer could add [$$observable], then any further enhancer changing getState would cause it to report incorrect values because the inner enhancer uses the inner getState.

Sealing off limits the domain of enhancers to:

  • Hijacking reducer
  • Hijacking initialState
  • Hijacking dispatch
  • Hijacking getState
  • Hijacking onChange

All these things are guaranteed to be orthogonal: it is always possible to write an enhancer that changes some of them and it is guaranteed that this won’t trip up enhancers before or after it.

However as soon as we let enhancers define non-orthogonal methods (such as [$$observable] which depends on subscribe and getState), this introduces subtle failures. If a default enhancer included subscribe, a thing that wants to delay subscribe would have to reimplement its logic.

I feel good that all existing use cases seem covered by (reducer, initialState, onChange) => { dispatch, getState }. I would be wary of making the system more expressive because this caused issues in the past. Most enhancers so far had to work around this freedom rather than enjoy it. Finally, for what it’s worth, it is always possible to really wrap createStore if you really need it. I’m just not convinced this is any close to being a common case for the enhancers I have seen.

@gaearon

gaearon May 9, 2016

Collaborator

The problem I tried to solve is that anything other than { getState, dispatch } is hard to compose, and enhancers have no knowledge of the past enhancers. If any enhancer could add a method like subscribe(), it would be a pain to later batch subscriptions in any later enhancer (this is pretty much why https://github.com/tappleby/redux-batched-subscribe is currently using a less performant subscription mechanism which was copy pasted from earlier versions of Redux). If any enhancer could add [$$observable], then any further enhancer changing getState would cause it to report incorrect values because the inner enhancer uses the inner getState.

Sealing off limits the domain of enhancers to:

  • Hijacking reducer
  • Hijacking initialState
  • Hijacking dispatch
  • Hijacking getState
  • Hijacking onChange

All these things are guaranteed to be orthogonal: it is always possible to write an enhancer that changes some of them and it is guaranteed that this won’t trip up enhancers before or after it.

However as soon as we let enhancers define non-orthogonal methods (such as [$$observable] which depends on subscribe and getState), this introduces subtle failures. If a default enhancer included subscribe, a thing that wants to delay subscribe would have to reimplement its logic.

I feel good that all existing use cases seem covered by (reducer, initialState, onChange) => { dispatch, getState }. I would be wary of making the system more expressive because this caused issues in the past. Most enhancers so far had to work around this freedom rather than enjoy it. Finally, for what it’s worth, it is always possible to really wrap createStore if you really need it. I’m just not convinced this is any close to being a common case for the enhancers I have seen.

if (typeof enhancer !== 'function') {
throw new Error('Expected the enhancer to be a function.')
function dispatch(action) {
if (!isPlainObject(action)) {

This comment has been minimized.

@acdlite

acdlite May 9, 2016

Collaborator

Could I suggest that you move all these opinionated checks into createStore() instead? Seems more appropriate there.

@acdlite

acdlite May 9, 2016

Collaborator

Could I suggest that you move all these opinionated checks into createStore() instead? Seems more appropriate there.

This comment has been minimized.

@gaearon

gaearon May 9, 2016

Collaborator

This way enhancers/middleware would be open to dispatch anything which can cause bugs (e.g. dispatching empty values by mistake). IMO they’re more useful here.

@gaearon

gaearon May 9, 2016

Collaborator

This way enhancers/middleware would be open to dispatch anything which can cause bugs (e.g. dispatching empty values by mistake). IMO they’re more useful here.

This comment has been minimized.

@acdlite

acdlite May 9, 2016

Collaborator

Yeah rethinking this, that's fine with me. If someone (me) really wanted to opt-out of those things, they could easily write their own createStoreBase().

But part of me does think it would be nice if there were a "core enhancer" that implemented this stuff, and was automatically applied by createStore. Then createStoreBase could be exported separately (contingent on finding a better name for it), which would allow for alternative createStore-like APIs to be experimented on in userland without having to rewrite much of anything.

@acdlite

acdlite May 9, 2016

Collaborator

Yeah rethinking this, that's fine with me. If someone (me) really wanted to opt-out of those things, they could easily write their own createStoreBase().

But part of me does think it would be nice if there were a "core enhancer" that implemented this stuff, and was automatically applied by createStore. Then createStoreBase could be exported separately (contingent on finding a better name for it), which would allow for alternative createStore-like APIs to be experimented on in userland without having to rewrite much of anything.

This comment has been minimized.

@acdlite

acdlite May 9, 2016

Collaborator

E.g. someone could write a createObservableStore (use any stream/event system here, just providing an example) that is used in place of createStore but is totally compatible with enhancers. Yes, it's possible anyway by composing createStore but this way is cleaner.

@acdlite

acdlite May 9, 2016

Collaborator

E.g. someone could write a createObservableStore (use any stream/event system here, just providing an example) that is used in place of createStore but is totally compatible with enhancers. Yes, it's possible anyway by composing createStore but this way is cleaner.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

The more I think about this the more I really like this, Dan. I hope we can think of a good name for "store base." In my view, a "store base" is really the meat of Redux, whereas what we now think of as a "store" is a store base plus some subscription stuff that really isn't all that important (other than as an an integration point for projects like React Redux), plus some extra opinions (actions should be serializable, automatic INIT actions) that are advisable for the majority of people, but in the grand scheme of things aren't super crucial either.

Collaborator

acdlite commented May 9, 2016

The more I think about this the more I really like this, Dan. I hope we can think of a good name for "store base." In my view, a "store base" is really the meat of Redux, whereas what we now think of as a "store" is a store base plus some subscription stuff that really isn't all that important (other than as an an integration point for projects like React Redux), plus some extra opinions (actions should be serializable, automatic INIT actions) that are advisable for the majority of people, but in the grand scheme of things aren't super crucial either.

@jokeyrhyme

This comment has been minimized.

Show comment
Hide comment
@jokeyrhyme

jokeyrhyme May 9, 2016

So, effectively, Redux comes with a default enhancer that adds the subscribe() method.

jokeyrhyme commented May 9, 2016

So, effectively, Redux comes with a default enhancer that adds the subscribe() method.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

@jokeyrhyme Yeah pretty much

Collaborator

acdlite commented May 9, 2016

@jokeyrhyme Yeah pretty much

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

@jokeyrhyme Actually, the way I'd model it in my head is

// Pseudo-code, not literal functions
store = eventSystem(enhancers(storeBase))

where the store base handles state updates and createStore adds an event system.

Collaborator

acdlite commented May 9, 2016

@jokeyrhyme Actually, the way I'd model it in my head is

// Pseudo-code, not literal functions
store = eventSystem(enhancers(storeBase))

where the store base handles state updates and createStore adds an event system.

@johanneslumpe

This comment has been minimized.

Show comment
Hide comment
@johanneslumpe

johanneslumpe May 9, 2016

Collaborator

I can somewhat understand where the idea about sealing off the base comes from, but I think that this is a bit too opinionated. If someone wants to extend the store with custom methods, they should be able to do so. It might be useful to put a big warning there if you do that, but just disallowing out of the box seems like a harsh change.

Collaborator

johanneslumpe commented May 9, 2016

I can somewhat understand where the idea about sealing off the base comes from, but I think that this is a bit too opinionated. If someone wants to extend the store with custom methods, they should be able to do so. It might be useful to put a big warning there if you do that, but just disallowing out of the box seems like a harsh change.

@jokeyrhyme

This comment has been minimized.

Show comment
Hide comment
@jokeyrhyme

jokeyrhyme May 9, 2016

We have replaceReducer, do you think there is a case for a replaceEnhancer, or otherwise dynamically attaching enhancers after the creation of the store?

It might be nice for performance, for example, to only attach the dev-tools enhancer while the dev-tools panel is visible. Or to be able to instantiate and attach a history enhancer after the creation of the store.

jokeyrhyme commented May 9, 2016

We have replaceReducer, do you think there is a case for a replaceEnhancer, or otherwise dynamically attaching enhancers after the creation of the store?

It might be nice for performance, for example, to only attach the dev-tools enhancer while the dev-tools panel is visible. Or to be able to instantiate and attach a history enhancer after the creation of the store.

@yelouafi

This comment has been minimized.

Show comment
Hide comment
@yelouafi

yelouafi May 9, 2016

I think this is far better than the actual way. Dispatching the INIT action after store creation 'and' using the enhanced dispatch would solve a bunch of startup issues like #1240 or other similar issues.

A small remark on the ActionTypes.INIT. The action is not actually exposed as a public API. Shouldn't be exposed as a kind of 'lifecycle event' ?

For example, to resolve issues like #1240 the middleware should wait first for the INIT action before dispatching its first action.

yelouafi commented May 9, 2016

I think this is far better than the actual way. Dispatching the INIT action after store creation 'and' using the enhanced dispatch would solve a bunch of startup issues like #1240 or other similar issues.

A small remark on the ActionTypes.INIT. The action is not actually exposed as a public API. Shouldn't be exposed as a kind of 'lifecycle event' ?

For example, to resolve issues like #1240 the middleware should wait first for the INIT action before dispatching its first action.

@zalmoxisus

This comment has been minimized.

Show comment
Hide comment
@zalmoxisus

zalmoxisus May 9, 2016

Like the concept a lot. Having onChange instead of subscribers will help to avoid a lot of edge cases, like the store being replaced by other enhancers and having the old state in our enhancer's subscriber.

The transition for Chrome extension would be a bit difficult as it is updated automatically, but keeping also old redux-devtools-instrument with deprecation warnings for the earlier Redux versions (probably when the third argument is not defined) should help.

zalmoxisus commented May 9, 2016

Like the concept a lot. Having onChange instead of subscribers will help to avoid a lot of edge cases, like the store being replaced by other enhancers and having the old state in our enhancer's subscriber.

The transition for Chrome extension would be a bit difficult as it is updated automatically, but keeping also old redux-devtools-instrument with deprecation warnings for the earlier Redux versions (probably when the third argument is not defined) should help.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

@acdlite

Why wouldn't one of today's enhancers continue to work the old way with this new createStore(), which has the same signature as before?

Today’s enhancers would work. However we can throw inside createStore if the enhancer returned any other methods than { getState, dispatch }. This way, if the enhancers want to keep working with the modern API (the only one we document right now), they will be forced to drop support for the old API. Since the majority of people now pass enhancers as an argument, enhancers will be pressured to implement compat with 4.x for the modern way.

In other words, today’s 3.x enhancers technically can work on 4.x stores, but as people will ask the authors to make them passable as third arguments, we’ll end up with 4.x-only enhancers that can only be applied as an argument because they only return { dispatch, getState } as enforced by createStore().

Collaborator

gaearon commented May 9, 2016

@acdlite

Why wouldn't one of today's enhancers continue to work the old way with this new createStore(), which has the same signature as before?

Today’s enhancers would work. However we can throw inside createStore if the enhancer returned any other methods than { getState, dispatch }. This way, if the enhancers want to keep working with the modern API (the only one we document right now), they will be forced to drop support for the old API. Since the majority of people now pass enhancers as an argument, enhancers will be pressured to implement compat with 4.x for the modern way.

In other words, today’s 3.x enhancers technically can work on 4.x stores, but as people will ask the authors to make them passable as third arguments, we’ll end up with 4.x-only enhancers that can only be applied as an argument because they only return { dispatch, getState } as enforced by createStore().

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

@timdorr

That is, rather than the enhancer extending the Redux "class", Redux inherits from something that implements a particular interface. (Not sure if that analogy is helpful, but it's what springs to mind at the moment).

If we were to draw a class analogy, then Store constructor accepts the enhancer “inheritance chain” as an argument, applies it to the StoreBase, and then contains an instance of FinalStoreBase while implementing a superset of StoreBase API, and delegates to it. It recreates the instance of StoreBase (with the whole chain) during replaceReducer.

Collaborator

gaearon commented May 9, 2016

@timdorr

That is, rather than the enhancer extending the Redux "class", Redux inherits from something that implements a particular interface. (Not sure if that analogy is helpful, but it's what springs to mind at the moment).

If we were to draw a class analogy, then Store constructor accepts the enhancer “inheritance chain” as an argument, applies it to the StoreBase, and then contains an instance of FinalStoreBase while implementing a superset of StoreBase API, and delegates to it. It recreates the instance of StoreBase (with the whole chain) during replaceReducer.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

@johanneslumpe

If someone wants to extend the store with custom methods, they should be able to do so.

They are able to, but since this breaks composition of enhancers, they would do it outside our API.
You can always do

store.myCustomWhatever = ...

It just doesn’t need we need to bless this anymore because we found the sweet extension point that solves most of the use cases I have seen in the wild with enhancers.

Collaborator

gaearon commented May 9, 2016

@johanneslumpe

If someone wants to extend the store with custom methods, they should be able to do so.

They are able to, but since this breaks composition of enhancers, they would do it outside our API.
You can always do

store.myCustomWhatever = ...

It just doesn’t need we need to bless this anymore because we found the sweet extension point that solves most of the use cases I have seen in the wild with enhancers.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

@jokeyrhyme

We have replaceReducer, do you think there is a case for a replaceEnhancer, or otherwise dynamically attaching enhancers after the creation of the store?

I think we wouldn’t be able to add this because there is no guarantee that the existing state generated by one enhancer tree would be compatible with the next enhancer tree. (Indeed, in the case of switching DevTools off, it wouldn’t be.) At this point you might as well create the store again directly.

Collaborator

gaearon commented May 9, 2016

@jokeyrhyme

We have replaceReducer, do you think there is a case for a replaceEnhancer, or otherwise dynamically attaching enhancers after the creation of the store?

I think we wouldn’t be able to add this because there is no guarantee that the existing state generated by one enhancer tree would be compatible with the next enhancer tree. (Indeed, in the case of switching DevTools off, it wouldn’t be.) At this point you might as well create the store again directly.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

@yelouafi

A small remark on the ActionTypes.INIT. The action is not actually exposed as a public API. Shouldn't be exposed as a kind of 'lifecycle event'? For example, to resolve issues like #1240 the middleware should wait first for the INIT action before dispatching its first action.

Yeah, I haven’t thought much about this yet. With this model we’re in a much better place to enforce constraints (e.g. we can throw if the middleware failed to synchronously pass INIT down). However we’ll need to think more about what behavior makes sense for the initial actions.

Collaborator

gaearon commented May 9, 2016

@yelouafi

A small remark on the ActionTypes.INIT. The action is not actually exposed as a public API. Shouldn't be exposed as a kind of 'lifecycle event'? For example, to resolve issues like #1240 the middleware should wait first for the INIT action before dispatching its first action.

Yeah, I haven’t thought much about this yet. With this model we’re in a much better place to enforce constraints (e.g. we can throw if the middleware failed to synchronously pass INIT down). However we’ll need to think more about what behavior makes sense for the initial actions.

@tomkis

This comment has been minimized.

Show comment
Hide comment
@tomkis

tomkis May 9, 2016

Contributor

I definitely like this change, looks like a clear separation of concerns. Tried to think about it in all my enhancers and didn't come with any potential issue.

Contributor

tomkis commented May 9, 2016

I definitely like this change, looks like a clear separation of concerns. Tried to think about it in all my enhancers and didn't come with any potential issue.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

By the way this gives us an opportunity to finally pass the state as an argument to the listeners. 😄
We now have a guarantee that it will be the final state provided by enhancers by the time we call them.

Collaborator

gaearon commented May 9, 2016

By the way this gives us an opportunity to finally pass the state as an argument to the listeners. 😄
We now have a guarantee that it will be the final state provided by enhancers by the time we call them.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

Copying part of my comment above here in case it gets hidden by a new commit:

But part of me does think it would be nice if there were a "core enhancer" that implemented this stuff, and was automatically applied by createStore. Then createStoreBase could be exported separately (contingent on finding a better name for it), which would allow for alternative createStore-like APIs to be experimented on in userland without having to rewrite much of anything.

Another way of putting this is that I think "store base" is a better way for userland to experiment with alternative (perhaps higher-level) APIs, rather than composing createStore. Aside from implementing different event systems (which could still be compatible with React Redux, as long as they expose the same methods), they could also do things like apply default enhancers and so forth in a cleaner, safer way.

Collaborator

acdlite commented May 9, 2016

Copying part of my comment above here in case it gets hidden by a new commit:

But part of me does think it would be nice if there were a "core enhancer" that implemented this stuff, and was automatically applied by createStore. Then createStoreBase could be exported separately (contingent on finding a better name for it), which would allow for alternative createStore-like APIs to be experimented on in userland without having to rewrite much of anything.

Another way of putting this is that I think "store base" is a better way for userland to experiment with alternative (perhaps higher-level) APIs, rather than composing createStore. Aside from implementing different event systems (which could still be compatible with React Redux, as long as they expose the same methods), they could also do things like apply default enhancers and so forth in a cleaner, safer way.

function replaceReducer(nextReducer) {
if (typeof nextReducer !== 'function') {
throw new Error('Expected the nextReducer to be a function.')
}
currentReducer = nextReducer
var nextInitialState = storeBase ? getState() : initialState
storeBase = createFinalStoreBase(nextReducer, nextInitialState, onChange)

This comment has been minimized.

@acdlite

acdlite May 9, 2016

Collaborator

Hey, now we can throw here if the enhancer returns the wrong thing! Sweet

@acdlite

acdlite May 9, 2016

Collaborator

Hey, now we can throw here if the enhancer returns the wrong thing! Sweet

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

Yeah, I haven’t thought much about this yet. With this model we’re in a much better place to enforce constraints (e.g. we can throw if the middleware failed to synchronously pass INIT down).

This is a good example of something that's fine to put inside createStore (and as part of a "core enhancer" as I suggest above) but I would really want a way to opt-out of — which exposing createStoreBase would allow. I hate the INIT action, and in some of my projects I use an enhancer to block it. Maybe this is irrational (I don't think it is) but for nuttier, "out there" projects of which I am fond, the INIT action is a pain in the ass. Separating this type of opinion from createStoreBase would make me and other tinkerers happy. Or maybe just me. I think it's a good idea either way :D

Collaborator

acdlite commented May 9, 2016

Yeah, I haven’t thought much about this yet. With this model we’re in a much better place to enforce constraints (e.g. we can throw if the middleware failed to synchronously pass INIT down).

This is a good example of something that's fine to put inside createStore (and as part of a "core enhancer" as I suggest above) but I would really want a way to opt-out of — which exposing createStoreBase would allow. I hate the INIT action, and in some of my projects I use an enhancer to block it. Maybe this is irrational (I don't think it is) but for nuttier, "out there" projects of which I am fond, the INIT action is a pain in the ass. Separating this type of opinion from createStoreBase would make me and other tinkerers happy. Or maybe just me. I think it's a good idea either way :D

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite May 9, 2016

Collaborator

@gaearon If you want, Dan, I'll submit a PR to this branch sometime today illustrating what I mean.

Collaborator

acdlite commented May 9, 2016

@gaearon If you want, Dan, I'll submit a PR to this branch sometime today illustrating what I mean.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

I'll submit a PR to this branch sometime today illustrating what I mean.

Sure!

Collaborator

gaearon commented May 9, 2016

I'll submit a PR to this branch sometime today illustrating what I mean.

Sure!

@tappleby

This comment has been minimized.

Show comment
Hide comment
@tappleby

tappleby May 9, 2016

Contributor

@gaearon This looks great!

Much better then having to duplicate the subscription logic or split into its own package, can probably even deprecate redux-batched-subscribe with this change since it pretty much becomes:

(createStoreBase) => (reducer, initialState, onChange) => createStoreBase(reducer, initialState, debounce(onChange))

The batched subscribe package does have a subscribeImmediate function which can be used to bypass the batching for some subscribers. This wouldn't be possible with the new structure but not sure if it was even worth having in the first place (could probably end up with some weird bugs when used)

Contributor

tappleby commented May 9, 2016

@gaearon This looks great!

Much better then having to duplicate the subscription logic or split into its own package, can probably even deprecate redux-batched-subscribe with this change since it pretty much becomes:

(createStoreBase) => (reducer, initialState, onChange) => createStoreBase(reducer, initialState, debounce(onChange))

The batched subscribe package does have a subscribeImmediate function which can be used to bypass the batching for some subscribers. This wouldn't be possible with the new structure but not sure if it was even worth having in the first place (could probably end up with some weird bugs when used)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 9, 2016

Collaborator

@tappleby I’d still keep it as a package for people who don’t want to dive into creating even a simple enhancer.

Collaborator

gaearon commented May 9, 2016

@tappleby I’d still keep it as a package for people who don’t want to dive into creating even a simple enhancer.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 18, 2016

Collaborator

Um, can we just dispatch @@INIT only if getState() is undefined by the time store is created? Reducers shouldn’t care which action gets dispatched initially anyway because Redux asks that you never handle @@INIT action directly (and we can randomize its name). If some middleware already fired some other action, so be it, we just won’t fire @@INIT. What do you think? @acdlite @yelouafi @timdorr

Collaborator

gaearon commented May 18, 2016

Um, can we just dispatch @@INIT only if getState() is undefined by the time store is created? Reducers shouldn’t care which action gets dispatched initially anyway because Redux asks that you never handle @@INIT action directly (and we can randomize its name). If some middleware already fired some other action, so be it, we just won’t fire @@INIT. What do you think? @acdlite @yelouafi @timdorr

@yelouafi

This comment has been minimized.

Show comment
Hide comment
@yelouafi

yelouafi May 18, 2016

I don't 'feel' the idea of someone dispatching actions before @INIT. But from a practical POV it doesn't seem to have issues as long as the reducers are written using the default parameter style(function recuder(state={...}, action) { ... }.

This may break reducers making assumptions that an undefined state is always equivalent to an @INIT action. But as you said reducers shouldn't care which action gets dispatched initially. So from a pragmatic POV I don't see an issue with your proposition

yelouafi commented May 18, 2016

I don't 'feel' the idea of someone dispatching actions before @INIT. But from a practical POV it doesn't seem to have issues as long as the reducers are written using the default parameter style(function recuder(state={...}, action) { ... }.

This may break reducers making assumptions that an undefined state is always equivalent to an @INIT action. But as you said reducers shouldn't care which action gets dispatched initially. So from a pragmatic POV I don't see an issue with your proposition

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 24, 2016

Collaborator

Let’s call “store base” an atom? It exists to provide atomic updates. We can also say “store holds an atom with the current state” and stay technically correct.

Collaborator

gaearon commented May 24, 2016

Let’s call “store base” an atom? It exists to provide atomic updates. We can also say “store holds an atom with the current state” and stay technically correct.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr May 24, 2016

Member

Maybe just call it a "store atom". It is a building block to make a full "store molecule" of sorts.

Member

timdorr commented May 24, 2016

Maybe just call it a "store atom". It is a building block to make a full "store molecule" of sorts.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jun 14, 2016

Collaborator

Not sure if we should be introducing new verbiage like "atom." A "store base" isn't fundamentally different from a store: both hold a value and notify when changes occur. "Store atom" is better, but still implies to me that it's a totally new concept, rather than a lower-level version of the same thing.

Collaborator

acdlite commented Jun 14, 2016

Not sure if we should be introducing new verbiage like "atom." A "store base" isn't fundamentally different from a store: both hold a value and notify when changes occur. "Store atom" is better, but still implies to me that it's a totally new concept, rather than a lower-level version of the same thing.

@jokeyrhyme

This comment has been minimized.

Show comment
Hide comment
@jokeyrhyme

jokeyrhyme Jun 14, 2016

We could just name this a "store", and come up with a new name for an "enhanced store"?

jokeyrhyme commented Jun 14, 2016

We could just name this a "store", and come up with a new name for an "enhanced store"?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 14, 2016

Collaborator

We could just name this a "store", and come up with a new name for an "enhanced store"?

The “enhanced store” would be what people work with in 99% cases. So it makes sense for it to stay “the store”.

Collaborator

gaearon commented Jun 14, 2016

We could just name this a "store", and come up with a new name for an "enhanced store"?

The “enhanced store” would be what people work with in 99% cases. So it makes sense for it to stay “the store”.

function replaceReducer(nextReducer) {
if (typeof nextReducer !== 'function') {
throw new Error('Expected the nextReducer to be a function.')
}
currentReducer = nextReducer
var nextInitialState = storeBase ? getState() : initialState

This comment has been minimized.

@jridgewell

jridgewell Sep 17, 2016

Contributor

With this, initialState will never be garbage collected. You'd either need to move the initial storeBase creation outside of replaceReducer, or null it out after this.

@jridgewell

jridgewell Sep 17, 2016

Contributor

With this, initialState will never be garbage collected. You'd either need to move the initial storeBase creation outside of replaceReducer, or null it out after this.

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Jan 3, 2017

Contributor

So I started cataloging store enhancers with the help of @markerikson. I've got this google spreadsheet in progress. I briefly looked at the code of all of these, to get a feel for what they're doing and how.

I think this PR would make a lot of these enhancers very unhappy. There's a lot of enhancers that add their own properties to the store, or do interesting things with the subscribe mechanics, as examples. Or a lot of them just subscribe. Taking those abilities away means they'll have to forego being a store enhancer and instead do their business outside of createStore, pushing the setup burden into the application code.

Contributor

jimbolla commented Jan 3, 2017

So I started cataloging store enhancers with the help of @markerikson. I've got this google spreadsheet in progress. I briefly looked at the code of all of these, to get a feel for what they're doing and how.

I think this PR would make a lot of these enhancers very unhappy. There's a lot of enhancers that add their own properties to the store, or do interesting things with the subscribe mechanics, as examples. Or a lot of them just subscribe. Taking those abilities away means they'll have to forego being a store enhancer and instead do their business outside of createStore, pushing the setup burden into the application code.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 3, 2017

Collaborator

do interesting things with the subscribe mechanics, as examples. Or a lot of them just subscribe.

Wouldn’t this still be possible with custom onChange? As far as I remember, this change doesn’t take away any capabilities from enhancers. It just makes those capabilities easier to implement since instead of shadowing the subscription mechanism you can just call onChange whenever.

Collaborator

gaearon commented Jan 3, 2017

do interesting things with the subscribe mechanics, as examples. Or a lot of them just subscribe.

Wouldn’t this still be possible with custom onChange? As far as I remember, this change doesn’t take away any capabilities from enhancers. It just makes those capabilities easier to implement since instead of shadowing the subscription mechanism you can just call onChange whenever.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 3, 2017

Collaborator

Could I ask you to pick a minimal enhancer example that seems inconvertible to the new approach?

Apart from custom methods/properties (IMO it’s a bad pattern and in this case it’s fine to force users to wrap, since otherwise they’d be looking for those methods/properties in Redux docs).

Collaborator

gaearon commented Jan 3, 2017

Could I ask you to pick a minimal enhancer example that seems inconvertible to the new approach?

Apart from custom methods/properties (IMO it’s a bad pattern and in this case it’s fine to force users to wrap, since otherwise they’d be looking for those methods/properties in Redux docs).

@Parakleta

This comment has been minimized.

Show comment
Hide comment
@Parakleta

Parakleta Sep 21, 2017

Contributor

I wasn't sure whether to start a new issue or comment here, but I think what I am struggling with is not possible in the current design of Redux and would need to be added as a 4.0 change. Essentially it is currently not possible to have middlewares emit actions that need to be processed by enhancers higher than them in the chain. The idea of a middleware emitting something that needs pre-processing before hitting the reducers appears to be accepted as a good thing because the middleware enhancer ensures that they are composed in a way that lifts all new actions to the top of the middleware chain. Giving enhancers access to the top-of-chain dispatch method ensures that new actions emitted by other enhancers are run through the full enhancer/middleware chain.

For middleware a distinction is made between next and dispatch, where next pushes the action further down the chain, and dispatch lifts it back up to the top. I would suggest that the new enhancer mechanism proposed here should have in addition to onChange another function such as liftAction which passes actions back up the chain. This is similar to the concept of the final store from #2214.

One additional advantage of this is that middleware no longer needs to be special, and is in fact just another enhancer.

Contributor

Parakleta commented Sep 21, 2017

I wasn't sure whether to start a new issue or comment here, but I think what I am struggling with is not possible in the current design of Redux and would need to be added as a 4.0 change. Essentially it is currently not possible to have middlewares emit actions that need to be processed by enhancers higher than them in the chain. The idea of a middleware emitting something that needs pre-processing before hitting the reducers appears to be accepted as a good thing because the middleware enhancer ensures that they are composed in a way that lifts all new actions to the top of the middleware chain. Giving enhancers access to the top-of-chain dispatch method ensures that new actions emitted by other enhancers are run through the full enhancer/middleware chain.

For middleware a distinction is made between next and dispatch, where next pushes the action further down the chain, and dispatch lifts it back up to the top. I would suggest that the new enhancer mechanism proposed here should have in addition to onChange another function such as liftAction which passes actions back up the chain. This is similar to the concept of the final store from #2214.

One additional advantage of this is that middleware no longer needs to be special, and is in fact just another enhancer.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Sep 21, 2017

Contributor

@Parakleta : I think the difference is that middleware can dispatch back to the start of the middleware pipeline. So, nothing is going outside the bounds of applyMiddleware itself. In addition, it's a deliberate part of the middleware design that they get both next and dispatch as references.

In contrast, enhancers are intended to be layered on top of each other without knowing what's before or after.

Can you give an example use case for when something like "going back to the outermost enhancer" might be necessary?

Contributor

markerikson commented Sep 21, 2017

@Parakleta : I think the difference is that middleware can dispatch back to the start of the middleware pipeline. So, nothing is going outside the bounds of applyMiddleware itself. In addition, it's a deliberate part of the middleware design that they get both next and dispatch as references.

In contrast, enhancers are intended to be layered on top of each other without knowing what's before or after.

Can you give an example use case for when something like "going back to the outermost enhancer" might be necessary?

@Parakleta

This comment has been minimized.

Show comment
Hide comment
@Parakleta

Parakleta Sep 21, 2017

Contributor

Essentially any time a middleware (or by extension an enhancer) emits a new action (in my case particularly async action emitters) I think those should be subject to the same enhancements as new actions emitted from other sources.

The absence of this ability is what has caused a range of issues to be raised regarding incompatibilities with the router enhancer and other middlewares, makes batching difficult, and I suspect is going to cause problems when redux-observable is converted from a middleware to an enhancer.

The issue is raised in #1051, #1240, and #1294 but kind of left unresolved. Related are all of the issues in other projects that cross-link with those issues. Quite a few of those external projects seem to be hoping that this PR will fix the problem for them.

In the very simplest case if I have a batching enhancer and some middleware if I compose them batch -> middleware then I cannot batch actions emitted by the middleware. If I compose them middleware -> batch then my batched actions must be in a form that can be ignored by the middleware but then after unbatching will not be processed by the middleware. I have seen a workable solution in https://github.com/abc123s/redux-batch-enhancer which unpacks the batched actions in a middleware and brackets them with PUSH/POP actions which a later enhancer uses to enable/disable batching, but this feels clumsier than it need be. With a top level dispatch it just becomes a matter of adding an enhancer with a function like:

var depth = 0;
function dispatch(action) {
    try {
        depth += 1;
        store.dispatch(action);
    } finally {
        depth -= 1;
    }
    if (depth === 0) {
        onChange();
    }
    return action;
}

I don't know. For every use case that I can probably come up with there is probably a work around anyway, but the crux of the issue is I think that the only dispatch available to enhancers is not actually dispatch, but rather next. If enhancers only manipulate actions then this is fine because they do their manipulation and pass it on, but if enhancers are allowed to generate actions then this restriction is not OK. If enhancers are not allowed to generate new actions then middleware shouldn't be an enhancer.

Contributor

Parakleta commented Sep 21, 2017

Essentially any time a middleware (or by extension an enhancer) emits a new action (in my case particularly async action emitters) I think those should be subject to the same enhancements as new actions emitted from other sources.

The absence of this ability is what has caused a range of issues to be raised regarding incompatibilities with the router enhancer and other middlewares, makes batching difficult, and I suspect is going to cause problems when redux-observable is converted from a middleware to an enhancer.

The issue is raised in #1051, #1240, and #1294 but kind of left unresolved. Related are all of the issues in other projects that cross-link with those issues. Quite a few of those external projects seem to be hoping that this PR will fix the problem for them.

In the very simplest case if I have a batching enhancer and some middleware if I compose them batch -> middleware then I cannot batch actions emitted by the middleware. If I compose them middleware -> batch then my batched actions must be in a form that can be ignored by the middleware but then after unbatching will not be processed by the middleware. I have seen a workable solution in https://github.com/abc123s/redux-batch-enhancer which unpacks the batched actions in a middleware and brackets them with PUSH/POP actions which a later enhancer uses to enable/disable batching, but this feels clumsier than it need be. With a top level dispatch it just becomes a matter of adding an enhancer with a function like:

var depth = 0;
function dispatch(action) {
    try {
        depth += 1;
        store.dispatch(action);
    } finally {
        depth -= 1;
    }
    if (depth === 0) {
        onChange();
    }
    return action;
}

I don't know. For every use case that I can probably come up with there is probably a work around anyway, but the crux of the issue is I think that the only dispatch available to enhancers is not actually dispatch, but rather next. If enhancers only manipulate actions then this is fine because they do their manipulation and pass it on, but if enhancers are allowed to generate actions then this restriction is not OK. If enhancers are not allowed to generate new actions then middleware shouldn't be an enhancer.

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