Alternate Proof of Concept: Enhancer Overhaul

[jimbolla]

Preface

This is an alternative solution to Proof of Concept: Enhancer Overhaul #1702 that attempts to achieve the same goals without constraining enhancers to a more limited API.

I've compiled a spreadsheet of store enhancer projects and examined most of their source code to get a sense of what those enhancers are currently doing.

Goals

1. Reduce the amount of code involved in writing an enhancer.
2. Give enhancers more ability to extend the base store functionality.
3. Limit/avoid breaking changes to the public API to avoid disrupting application developers.

Modern Enhancers

The major new concept is evolving the current enhancer API (which I will call "classic enhancers") to a new API (which I will call "modern enhancers"). Here's a simple example of a modern enhancer:

export default function logAllActions(store) {
  const actionLog = []

  function dispatch(action) {
    actionLog.push(action)
    return store.dispatch(action)
  }

  // This enhancers adds an actionLog property and overrides the dispatch method
  return { actionLog, dispatch }
}

A modern enhancer accepts a store and returns a new partial store object with the properties it adds + overrides. Conceptually, a modern enhancer is similar to the Decorator design pattern. Even the core store functionality has been refactored so that it's described as several modern enhancers.

createStore calls the sequence of enhancers to build the final store:

  const allEnhancers = [
    makeStoreInitializeStateViaDispatch,
    blockReducerFromAccessingStore,
    ...(modernEnhancers || []),
    makeStoreObservable,
    createBaseStore,
  ]
  const finalStore = allEnhancers.reduceRight((store, enhancer) => {
    // details 
  }, {})

This is the build phase of creating the store, in which all the enhancers add their behavior to the store. After the build phase, is the initialization phase, when createStore calls finalStore.init({ reducer, preloadedState }).

New: store.init

Since a modern enhancer doesn't have access to the initial reducer or preloaded state as part of its main method, its opportunity to intercept those values is now done in store.init. init is also the appropriate time to do things such as subscribing to the store or any external setup calls. For example redux-persist's autoRehydrate enhancer would change from this:

export default function autoRehydrate (config = {}) {
  const stateReconciler = config.stateReconciler || defaultStateReconciler
    
  return (next) => (reducer, initialState, enhancer) => {
    let store = next(liftReducer(reducer), initialState, enhancer)
    return {
      ...store,
      replaceReducer: (reducer) => {
        return store.replaceReducer(liftReducer(reducer))
      }
    }
  }

  function liftReducer(reducer) { // ...
  }
}

To this:

export default function autoRehydrate (config = {}) {
  const stateReconciler = config.stateReconciler || defaultStateReconciler

  return store => ({
    init(options) {
      store.init({ ...options, reducer: liftReducer(options.reducer) })
    },
    replaceReducer: (reducer) => {
      return store.replaceReducer(liftReducer(reducer))
    }
  })

  function liftReducer(reducer) { // ...
  }
}

New: store.final

By making createStore control the entire build process of the final store, the "magic" feature this enables is allowing all the enhancers to have a reference to the final store. This feature was inspired by applyMiddleware, and mechanically it works the same way. For example, the base store's dispatch method looks like:

function createBaseStore(store) {
  let currentReducer = uninitializedReducer
  let currentState = undefined

  function dispatch(action) {
    validateAction(action)
    currentState = store.final.reducer(currentState, action)
    store.final.onChange()
    return action
  }

  // ...

  return {
    dispatch,
    // ...
  }
}

The base dispatch method invokes the final versions of reducer and onChange, not just those methods as defined on the base store. This allows other store enhancers to override the default implementations called by dispatch. Currently in 3.6, overriding reducer requires wrapping it during createStore call and also replaceReducer. The logic for onChange is not overridable in 3.6 and requires an enhancer to wrap dispatch and subscribe.

New: store.reducer

A common pattern among existing enhancers is wrapping the reducer in a new function, while calling createStore/next and also in replaceReducer. This pattern can be simplified by putting reducer on the store. For example, here is the current version of install from redux-loop:

const { isNone, execute, batch } = require('./Cmd')

const install = () => (next) => (reducer, initialModel, enhancer) => {
  let queue = []

  const liftReducer = (reducer) => (state, action) => {
    const [model, cmd] = reducer(state, action)

    if (!isNone(cmd)) {
      queue.push(cmd)
    }

    return model
  }

  const store = next(liftReducer(reducer), initialModel, enhancer)

  const dispatch = (action) => {
    store.dispatch(action)

    if (queue.length) {
      const currentQueue = queue
      queue = []
      return execute(batch(currentQueue))
        .then((actions) => Promise.all(actions.map(dispatch)))
        .then(() => {})
    }

    return Promise.resolve()
  }

  const replaceReducer = (reducer) => {
    return store.replaceReducer(liftReducer(reducer))
  }

  return {
    ...store,
    dispatch,
    replaceReducer,
  }
}

module.exports = install;

And converted to a modern enhacer, taking advantage of the ability to override reducer:

const { isNone, execute, batch } = require('./Cmd')

const install = () => store => {
  let queue = []

  const reducer = (state, action) => {
    const [model, cmd] = store.reducer(state, action)

    if (!isNone(cmd)) {
      queue.push(cmd)
    }

    return model
  }

  const dispatch = (action) => {
    store.dispatch(action)

    if (queue.length) {
      const currentQueue = queue
      queue = []
      return execute(batch(currentQueue))
        .then((actions) => Promise.all(actions.map(dispatch)))
        .then(() => {})
    }

    return Promise.resolve()
  }

  return { dispatch, reducer }
}

module.exports = install;

New: store.onChange

Another desire of some store enhancers is the ability to control when subscriptions are notified after actions are dispatched. In order to make that simpler, base dispatch now invokes store.final.onChange(). Additionally the listener subscribe/onChange logic has been extracted into its own file createEvent. As an example, this allows redux-batched-subscribe to go from this:

export function batchedSubscribe(batch) {
  if (typeof batch !== 'function') {
    throw new Error('Expected batch to be a function.');
  }

  let currentListeners = [];
  let nextListeners = currentListeners;

  function ensureCanMutateNextListeners() { // ...
  }

  function subscribe(listener) { // ...
  }

  function notifyListeners() { // ...
  }

  function notifyListenersBatched() {
    batch(notifyListeners);
  }

  return next => (...args) => {
    const store = next(...args);
    const subscribeImmediate = store.subscribe;

    function dispatch(...dispatchArgs) {
      const res = store.dispatch(...dispatchArgs);
      notifyListenersBatched();
      return res;
    }

    return {
      ...store,
      dispatch,
      subscribe,
      subscribeImmediate
    };
  };
}

To this:

import { createEvent } from 'redux';

export function batchedSubscribe(batch) {
  if (typeof batch !== 'function') {
    throw new Error('Expected batch to be a function.');
  }

  return store => {
    const batchedOnChange = createEvent();

    return {
      onChange() {
        store.onChange();
        batch(batchedOnChange.invoke);
      },
      subscribe: batchedOnChange.subscribe,
      subscribeImmediate: store.subscribe,
    };
  };
}

Backwards compatibility

Some extra API is needed in order to prevent/minimize disruption of any API changes for application developers. First, because applications are using compose to combine enhancers and passing the result of that to createStore as the 3rd argument, an adapter is needed to make a modern enhancer backwards compatible. Two methods, adaptEnhancer and adaptEnhancerCreator are provided to make a modern enhancer match the signature of a classic enhancer.

Wrapping a modern enhancer would look like:

import { adaptEnhancer } from 'redux'

export default adaptEnhancer(function logAllActions(store) {
  const actionLog = []

  function dispatch(action) {
    actionLog.push(action)
    return store.dispatch(action)
  }

  // This enhancers adds an actionLog property and overrides the dispatch method
  return { actionLog, dispatch }
})

Wrapping a modern enhancer that has a factory function would look like:

import { adaptEnhancerCreator } from 'redux'

export default adaptEnhancerCreator(applyMiddleware)

function applyMiddleware(...middleware) {
  return store => {
    // ...
  }
}

Going Further: Simplifying Redux with breaking changes

Some things not part of this PR, but worth discussing...

Good idea: Drop support for compose + classic enhancer syntax completely

The need for adaptEnhancer, adaptEnhancerCreator, and compose could be removed if createStore drops support for the classic enhancer syntax. Some suggestions:

// style A: createStore(reducer, ...enhancers) - Simple and terse, but harder to extend.
const store = createStore(
  rootReducer,
  preloadState(preloadedState), // make an enhancer called preloadState
  applyMiddleware(thunk, api, createLogger()),
  DevTools.instrument(),
)

// style B: createStore(options) - All args are passed explicity by name. More verbose but
// more communicative. Easier to add new options in the future if the need arises.
const store = createStore({
  reducer: rootReducer,
  preloadedState: preloadedState,
  enhancers: [
    applyMiddleware(thunk, api, createLogger()),
    DevTools.instrument(),
  ] 
})

The benefits of doing so would be a decent reduction in code and the number of concepts that need supported/documented.

Moonshot: Middleware are made redundant by store.final and could be phased out

Redux could be made conceptually simpler if all middleware are then converted to store enhancers.

For example here's redux-thunk as middleware:

function createThunkMiddleware(extraArgument) {
  return ({ dispatch, getState }) => next => action => {
    if (typeof action === 'function') {
      return action(dispatch, getState, extraArgument);
    }

    return next(action);
  };
}

const thunk = createThunkMiddleware();
thunk.withExtraArgument = createThunkMiddleware;

export default thunk;

And here's redux-thunk as an enhancer:

function createThunkEnhancer(extraArgument) {
  return store => ({
    dispatch(action) {
      if (typeof action === 'function') {
        return action(store.final.dispatch, store.getState, extraArgument);
      }

      return store.dispatch(action);
    }
  })
}

const thunk = createThunkEnhancer();
thunk.withExtraArgument = createThunkEnhancer;

export default thunk;

The enhancer version has the advantage that store.final.dispatch references dispatch beyond just the one created by applyMiddleware; it is less "sandboxed." An ecosystem-wide refactoring to accomplish this for all middleware would be a very labor intensive operation, so I'm not sure it's worth it. Maybe suggest new projects write code as enhancers instead of middleware?

Wrapping up

Much like #1702, this is a pretty big change, in mostly the same ways; and most of the main bullet points from that one still apply. The key difference is this solution doesn't limit the enhancers ability to add/override properties to the store, which a good number of enhancers do.

[markerikson]

Definitely a lot to take in there.

Per your last item: while there's 50+ store enhancers out there, there's several hundred middlewares, and effectively every Redux project out there uses one or more middleware.. I don't think the "turn middlewares into enhancers" idea has any feasibility whatsoever.

Obligatory pinging of @gaearon , @acdlite , @timdorr , and all of @reactjs/redux for comment.

[tappleby]

One request that keeps coming up redux-batched-subscribe is having access to the action for applying some conditional logic on when to notify listeners. With the current onChange API this wouldn't be possible unless action gets passed along as well.

[markerikson]

@tappleby : that's really related to the intent of the core Redux subscription API itself. There's been numerous requests since the beginning to add either the action or the current state as an argument to subscription callbacks. Issue #580 has Dan's reasoning and a roundup of almost all the other times this question was asked.

That said, I'm vaguely starting to wonder if we at least ought to re-discuss the idea for a notional 4.0 release. I'd have to go back and review all the prior discussions, but I'm not immediately seeing actual harm that might happen if we implemented the idea.

[tappleby]

I still agree with #580, this behaviour is actually what makes a library like redux-batched-subscribe (RBS) possible, if the action was passed to subscription listeners this would make it difficult to perform any debounce or batching logic.

Keeping consistent with this API was my main reason not implementing the feature in RBS yet. But based on the number of requests to support conditional batching logic based on the action + the majority of the forks existing just to add this feature, I am re-evaluating things.

Does passing the action "internally" within redux to onChange run into the same/similar issues #580 is trying to avoid by passing the action to external subscription listeners?

[jimbolla]

I'm on the fence. I think, outside of the store, app code shouldn't depend on what actions were fired. But within a store enhancer or middleware, perhaps this isn't as true. Most middleware already is just doing something based on what action was dispatched. Perhaps an enhancer overriding onChange is no different. The risk is if 2 or more enhancers override dispatch/onChange in incompatible ways, the more coupled the method signatures, the more likely of an incompatibility.

[gaearon]

I'd have to go back and review all the prior discussions, but I'm not immediately seeing actual harm that might happen if we implemented the idea.

The reason it seemed dubious to me is I don’t think we even guarantee that all subscribers are called for all actions. I’m not sure I remember how we dispatch actions right now, but my understanding is if we dispatch action B while notifying subscribers about action A, we might as well notify the rest just about action B. It would be a bit odd to call subscribers twice (even if we do this now, not doing it seems like an optimization opportunity).

The intended guarantee is that Redux always eventually (within the top-level dispatch) calls all subscribers with the most recent state, but not that it always calls each subscriber for each action.

[gaearon]

Regarding this PR. I really appreciate all the research that went into it, but this is not quite the direction I imagined for Redux.

Right now Redux is not “integrated”, but at least its internals (and contracts) are simple. I’m worried this change makes it neither simple nor integrated, leaving it in an uncanny valley. The explicit goal here is to support existing characteristics of enhancers (such as mutating APIs) but I’m not sure they are actually good characteristics in long term. Maybe making some use cases “uglier” will force people to come up with better APIs instead of putting them on the store object.

I’m not actively involved in Redux anymore so my opinion is probably biased and not very relevant though.

[markerikson]

I’m not actively involved in Redux anymore so my opinion is probably biased and not very relevant though.

Aw, c'mon, Dan. Even you're not actively working on Redux itself these days, your opinions and insights are always valuable, informative, and welcomed.

Right now Redux is not “integrated”

Can you clarify what you mean by "integrated" here?

I've got a bunch of different things on my plate atm, so I don't have time right now to turn full attention to actual Redux change proposals. (Fortunately, there's no particular hurry). I'll toss out my general thoughts, though.

As I've said numerous times, my default answer to proposed Redux changes is "no", unless it's fixing an actual bug or adding an actually useful capability. That said, now that the core library has been stable for quite a while, I do think it's worth seeing how the ecosystem has evolved and taking that into account (ie, the "worn path" approach to laying down a sidewalk). I think investigating ways to make the ecosystem's job easier is a worthwhile effort, and it's also worth going back to commonly asked-for features like "action/state as a subscription argument" to at least re-evaluate them in light of where things stand now. I'm not saying we automatically say yes to all prior suggestions, just that there might be some stuff that is worth implementing after all now that we understand the use cases better.

[gaearon]

Another problem with passing the action (sorry that this discussion is unrelated to the PR!) is that it makes it harder to add debouncing in the future. The moment action is available, your app (or a third party library) begins to depend on it, and now we can’t safely debounce and batch subscriptions.

[markerikson]

Yeah, @tappleby just mentioned that in another comment. And yes, that's a great reason to back up the current stance against adding that to subscription callbacks, and I'll make a note to add that to the eventual FAQ entry on the topic.