RFC - StoreModifier, dynamically modifying store from component tree

[Ephem]

Warning: Do not merge!
For now, this PR is an RFC submitted for discussion only.

This PR is a continuation and updated version of #1150, also see #1126 for more background.

I am posting this as a PR to help more nuanced discussions and for better change tracking. The code changes proposed are an unfinished demo-implementation only, it has not been tested exhaustively and is currently lacking any unit/integration-tests.

Problem: Because ReactReduxContext is not part of the public API, there is currently no way using only public APIs to safely support dynamic loading of reducers (or modifying the store from inside the component tree). Even using ReactReduxContext, implementation is tricky, error prone and hard to get right in userland (especially with SSR) and there is also room to create a better story around code splitting reducers.

Proposed solution: User provides a modifyStore(modification, storeProxy, state)-function to <Provider>. New component <StoreModifier modification={modification}> can be used to wrap a subtree that needs dynamically loaded reducers (or other store modifications). HOC withModifiedStore can be used as an alternative to <StoreModifier>.

Full RFC as rendered text

Simple CodeSandbox example (Originally forked from @rgrove)

I welcome any and all comments, suggestions and feedback!

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 302e864

https://deploy-preview-1154--react-redux-docs.netlify.com

[Ephem]

Ping @mpeyper and @abettadapur, I would love your feedback on the updated RFC!

[abettadapur]

I am not a huge fan of having users write this function like this, perhaps we can go with something like the chaining method?

function storeModification1(modification, storeProxy, state) {
    ///whatever
}

function storeModification2(modification, storeProxy, state) {
    ///whatever 2
}

return (
    <Provider store={store} storeModifier={composeModifiers(storeModifier1, storeModifier2)}>...</Provider>
);

What do you think?

[Ephem]

I think I agree that chaining should be supported out of the box, though I'm not yet sure about the best API for that (a compose-function or supporting passing an array in)? Might be nice to support single functions as well for simple usecases.

The only unsolved issue I see with chaining is how to avoid clashes and collisions in the modification-object when multiple libs use it, do you have any ideas around that? :)

[mpeyper]

Personally, I prefer the compose function. I draw a few parallels with this RFC and middleware, which has the applyMiddleware function to compose the together. Perhaps the compose function should be called applyModifiers? Maybe not though as in the middleware case it is a store enhancer that applies the middleware, but in the store modifiers case it is in itself a store modifier. combineModifiers?

[Ephem]

I think I prefer that too! The implementation of the combine-function needs to look a bit different than the current combine in Redux because of how things are passed in, but that's manageable.

Going this route, the big question in my head is "Should modifiers actually be a part of core Redux?". If so, perhaps modifiers should actually be registered/applied directly to the store-object?

If we aim for this approach, this RFC should be split into two, one describing what a "modifier" is in Redux, and this one, that describes how these are applied safely from inside the render-tree in React-Redux. If so, I think it would make sense to explore "modifiers" as something that can also apply modifications at create-time. For example, registering initial sagas could be a "modification".

I'd be happy to split this up and formulate it into two RFCs, but I'll refrain from doing so until we've gotten further in the discussions and I've gotten broader feedback.

[mpeyper]

My feeling is that redux already has a fairly solid concept for these types of things in the enhancer/middleware functionality. I'm struggling to think of how modifiers would be different enough, or offer anything that the others don't already provide.

The only thing that is potentially missing is perhaps a more formal way of injecting values into middleware (i.e. running a new saga/observable) as each middleware currently has to implement this themselves and often they have their own, unique way of doing it. If there was a standard API for pushing values into middleware than I think the storeModifiers concept goes away and then there is just a component required to call that API during rendering.

Just spitballing, but the middleware could subscribe to modifications as part of their setup step:

function createSagaMiddleware (options = {}) {
  function runSaga (saga) {
    // ...
  }

  return (middlewareAPI) => { // usually just called `store

    middlewareAPI.acceptModification('@@redux-saga/RUN_SAGA', (modifier) => runSaga(modifier.saga);

    return (next) => (action) => {
      // ...
    }
  }
}

const store = createStore(reducer, applyMiddleware(createSagaMiddleware()))

store.modify({ type: '@@redux-saga/RUN_SAGA', saga }) 

There is a very strong similarity here between actions/dispatch/subscribe and modifier/modify/acceptModification, but it's important to note that they are different, both semantically and in the restrictions applied to them (e.g. actions should be serialisable, modifications would not need to be).

[Ephem]

This feels like a really interesting idea! 💡 I haven't thought about it deeply, but intuitively it makes a lot of sense.

I'm starting to think that treating "modifications to middleware" and "replacing the root reducer" as the same problem and cramming them into a single API might be a mistake after all.. There is definitely worth in doing them both from the same utility-component/HOC (<StoreModifier> for example), but should we really think about them as the same kind of problem implementation-wise? Also see this comment.

[abettadapur]

I think the rest of this looks quite good, thanks @Ephem!

[mpeyper]

Personally, I prefer the compose function. I draw a few parallels with this RFC and middleware, which has the applyMiddleware function to compose the together. Perhaps the compose function should be called applyModifiers? Maybe not though as in the middleware case it is a store enhancer that applies the middleware, but in the store modifiers case it is in itself a store modifier. combineModifiers?

[mpeyper]

If store.getState() is unsafe here, could we instead patch the storeProxy argument to replace the getState function to return this state value instead of calling the store?

It could also return the updated state (that will be replaced in the context after replaceReducers is called.

[Ephem]

Well, it can be, but is not necessarily. I can imagine there being cases where you DO want to get the latest store state and not the one on this render. I hesitate changing the semantics of store.getState(), I think it would be confusing if it did not always get the latest state from store? (Unless we made it really clear that storeProxy is a proxy that works differently, but if we start changing the behaviour of functions to be "context-safe", the expectation would probably be that all functions would be safe, something I don't think we can guarantee)

I do agree it would be nice if you could get the resulting state from replaceReducer though. store.getState() after calling replaceReducer returns updated state, but that might contain other changes as well.

I think there is a tradeoff we need to consider carefully here, between "just use store and we'll try to patch things up magically behind the scenes" and being more explicit about what is happening. I'm starting to lean towards providing replaceReducer as a separate parameter instead so it is clear it is special, but also provide the unmodified store as an extra parameter for those that need it, but make it clear it is potentially unsafe to use. If it is clear to the user that replaceReducer is special, it could also return the new "safe" state.

[mpeyper]

IMO, the store has 4 functions by default: getState, dispatch, subscribe and replaceReducer.

• getState: not "context-safe"
• dispatch: "context-safe" so long as you don't try to use the updated state
• subscribe: not "context-safe" but I agree we should strip it out of the storeProxy anyway
• replaceReducer: "context-safe" if getState returns the patched state after the reducer is added

Any store enhancers that add additional functions to the store would need be responsible for ensuring its context safety itself.

I feel that adding the unmodified store but noting it's unsafeness no better than accessing it from the "private" context and it will be easy to abuse and get wrong and become a footgun for users.

[mpeyper]

Option 1:

Don't. Most things in redux (actions, middleware, basic reducers) don't deal with namespacing out-of-the-box. Leave it up the the users/library authors to manage it.

Option 2:

Perhaps if we use the proposed compose function above, we could use it to pass in slices of the options, similarly to combineReducers and state slices:

function dynamicReducers(modification, storeProxy, state) {
    // modification === modification.reducers
}

function dynamicSagas(modification, storeProxy, state) {
    // modification === modification.sagas
}

return (
    <Provider store={store} storeModifier={combineModifiers({ reducers: dynamicReducers, sagas: dynamicSagas })}>...</Provider>
);

Option 3:

Something else?

[Ephem]

I'm leaning towards Option 1, but with the caveat of providing advice along these lines in the documentation (better formulated..):

When using multiple modifiers, or if you are a library author, we recommend creating utility-functions for creating the modification-object, for example: addReducersModification(newReducers). This should work as an action-creator and could look something like this:

const addReducersModification = newReducers => ({
    type: '@your-library-name/type-of-modification',
    payload: newReducers
});

Then make sure your modifier-functions only respond to the modifications with the correct type.

The modification-object really is very similar to an action, just describes a modification supposed to happen to the store instead. For actions we handle namespacing via type, so why not reuse that concept here?

It would of course be possible to rename modification to action instead, but I think that might be more confusing than it clarifies things?

[mpeyper]

Should this also include a modifyStore proxy to allow modifier from calling other modifiers?

let storeProxy

const modifyStoreProxy = (modification) => modifyStore(modification, storeProxy, storeState)

storeProxy = { ...store, replaceReducer: replaceReducerSpy: modifyStore: modifyStoreProxy }

function dynamicReducers(modification, storeProxy, state) {
  // whatever
}

function addAdminReducer(modification, storeProxy, state) {
  if (state.user.isAdmin) {
    storeProxy.modifyStore({ reducers: [adminReducer] })
  }
}

return (
    <Provider store={store} storeModifier={combineModifiers({ reducers: dynamicReducers, addAdminReducer })}>...</Provider>
);

Looking at that above example, I can see why we may not want to use a combineReducers like API to solve the namespacing issue. Not all modifiers will necessarily require a slice of the modifications object.

[Ephem]

Hmm, that's interesting. If we want modifiers to be truly modular and composable I think we'd need something like this. If we do add the modifiers-concept to Redux directly, that function will probably already exist on the store.

[Ephem]

Thanks for some great comments. 👏 I wanted to add a few general thoughts around composing/chaining modifiers.

First of all, I like it! While it has some unsolved questions and can probably be improved upon, I think there might be a pretty strong and flexible API and concept here. If each library (like redux-saga, redux-observable etc) could easily provide their own modifiers, it would simplify things across the board. It's a great fit with any component model really, since often specific reducers, sagas, epics etc are only needed in parts of an application, but this is pretty hard to achieve today.

My one worry is that it might grow the API, implementation and complexity by quite a bit. It definitely feels like scope creep from the original problem. My initial feeling is that if we do want to add this, it should be added directly to Redux and that to be worth adding such functionality directly into core, the case needs to be pretty strong.

If we could further explore making a "modification" a first class citizen in the sense that they can be used to simplify the API for creating the store, that would definitely increase the value imo.

I'm starting to think that there are three ways forward:

1. Pause this RFC and explore the concept of adding an API around modifiers first
2. Grow this RFC and deal with both things at the same time
3. Shrink this RFC back down to only dealing with replacing reducers (and explore more general modifiers later)

I worry we are currently biting off a bit more than we can chew comfortably.. This is the point where I'd really love some directional feedback from the core maintainers. 😄

[mpeyper]

I agree that the core idea has grown, but I think it's necessary if we want to improve the code-splitting story in redux. IMO, redux thrives on its ecosystem, and not at least planning for more general support will see us quickly build ourselves into a corner.

I do agree that without input and direction from the core maintainers, any more work into this is likely to be wasted.

I'm not sure you will get much buy in from @timdorr on adding a new concept to redux core. He was pretty firm about not wanting to include some fairly standard utilities and actually wanting to remove more from the core package. I would like to hear his take on all of this, and of course @markerikson's as well.

[Ephem]

I agree, but improving the code splitting story was actually a secondary objective of this RFC. 😄 My feeling is also that we are building ourselves into a corner with this RFC exactly because we are trying to cater too much to the entire ecosystem. 😉 (Though I like your idea of acceptModification..)

The original problem this RFC set out to solve was to add a minimal building block for using replaceReducer safely from the component tree, considering the level of complexity that was added for doing this in a futureproof way in userland in v6. While it is very debatable, I would personally argue that since reducers and replaceReducer are core concepts of Redux, the official React-bindings should provide a safe way to interact with these from React. This is also one thing that everyone needs for code splitting.

Another problem, which is related but separable, is how to ergonomically modify middleware, both with and without React. As you mentioned, there is currently no API in (React-)Redux for this (but several existing in userland), maybe there should be, and I'm very glad we're talking about it and exploring the space for precisely the reasons you mention.

That said, I do think this RFC is currently trying to tackle too much to get off the ground. Also, by narrowing it back down from what it is now to tackle a single problem, I think chances are better we can provide a minimal but flexible API to build other/future solutions on top of, this applies to both userland and any future changes to (React-)Redux. I'll give this some more thought.

Even a minimal API like that would likely take a long time to evaluate and implement, considering the uncertainties of concurrent rendering etc. Meanwhile, I will try to find some time to document the problem and solution in a more concise way to help out with userland implementations. 📜

[Ephem]

I'm going to go ahead and close this PR and the related issue since there are more important things to tackle short-term and things will need time to stabilise before revisiting this.

For more details, see the related issue.

[markerikson]

FWIW, I definitely appreciate the detailed thought that went into this idea :) But yes, I think we've got other more important concerns atm, and there's not an immediate urgent need to try to get functionality like this directly into React-Redux itself atm.

[Ephem]

That's worth a lot, thanks for saying so! I totally agree and still think the discussions here and in the issue will be valuable when and if this is revisited, so I'm more than happy with the outcome. :)

I'll try to come back with a condensed form of all this, adapted to any new implementation, in the future.