RFC - API for dynamically adding reducers

[Ephem]

See discussion in issue #1126 for context.

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. Even using this, implementation is tricky and error prone and there is also room to create a better story around code splitting reducers.

Proposed solution: User provides a getRootReducer(newReducers)-function to <Provider>. New component <WithReducers reducers={newReducers}> can be used to wrap a subtree that needs these reducers.

---

Summary of problem (recap of #1126)

react-redux version 6 now uses the new context instead of the legacy one, and instead of every connected component being subscribed directly to the store, only the root <Provider> is. That is:

• Before: Every connected component called getState()
• After: Only <Provider> calls getState(), state gets passed along via context and does not change during a single render

This is done to avoid “tearing” in concurrent rendering mode (avoid different parts of application rendering with different state).

In a world with concurrent rendering, replaceReducer is trickier to use from within the render tree. This is because a call to this function modifies the state in the store and subsequent components expect this state to be available, but now no longer fetches the latest state automatically, but instead from context.

On top of fetching store from the context and calling replaceReducer on it, the solution from #1126 is to patch the current storeState from context with the state added by the new reducers by wrapping that subtree in a new <ReactReduxContext.Provider>. However, ReactReduxContext is not part of the public API.

In a sense this is not a problem new to v6, since several of the old solutions for code splitting reducers in React relied on fetching store directly from context, which was never part of the public API.

On top of this, I feel like code splitting is something to be encouraged and there is room to improve the story around this and that this aligns with the goals of react-redux.

Proposed solution

Please note that this is just a rough sketch of what a solution could look like. I’d love feedback and to get a discussion going around both large issues and small.

Rough implementation and example usage in CodeSandbox - This is forked from @rgrove’s excellent repro-case from #1126

My hope is that this solution strikes a balance between being immediately useful and preserving the flexibility for the user to construct a root-reducer and injection techniques however they feel like.

Goals:

• Add API for adding reducers from the component tree
• Do not require direct use of store or ReactReduxContext in userland
• Flexible API that supports both simple and complex usecases well and can be built on top of
• Small surface area
• Aligned with react-redux v6 approach for preparing for concurrent-safeness

Example usage

(Some imports etc omitted for brevity)

import { WithReducers } from 'react-redux';
import reducer from './namesReducer';

function Greeter({ currentName }) {
    return `Hi ${currentName}!`;
}

const mapStateToProps = state => ({
    currentName: state.names.currentName
});

const ConnectedGreeter = connect(mapStateToProps)(Greeter);

const newReducers = {
    names: reducer
};

export default props => (
    <WithReducers reducers={newReducers}>
        <ConnectedGreeter {...props} />
    </WithReducers>
);

import staticReducers from './staticReducers';

// Example simple userland implementation
let reducers = { ...staticReducers };
function getRootReducer(newReducers) {
    reducers = { ...reducers, ...newReducers };
    return combineReducers(reducers);
}

const DynamicGreeter = React.lazy(() => import('./Greeter'));

ReactDOM.render(
    <Provider store={store} getRootReducer={getRootReducer}>
        <Suspense fallback={<div>Loading...</div>}>
            <DynamicGreeter />
        </Suspense>
    </Provider>

API

Pretty much what you see in the example above, with the addition that <WithReducers> also takes the boolean prop replaceExisting which determines if existing reducers/state gets overwritten. Defaults to false.

Could also add other things, such as optionally removing reducers/state on unmount.

Unanswered questions

• Naming (Is getRootReducer really a good name considering it’s basically expected to have sideeffects? Should WithReducer be called something else? Etc..)
• When we have a component like this, it’s easy to add a HOC based on it, should this be part of the official implementation? Component version leaves forwardRef and hoisting statics to userland, HOC could do this behind the scenes.
• This solution does not take into account other sideeffects such as adding sagas etc. Might be possible to add those in userland in getRootReducer since you often have access to store there, but then it should be renamed. Not sure if it’s in scope for this RFC.
• Optionally removing reducers on unmount?

Probably a lot of questions I haven’t thought about, please feel free to add more!

Alternative solutions

Status quo - Determine that it is fine that these solutions rely on non-public APIs

Not desirable. Code splitting should not break between versions.

Make ReactReduxContext part of public API - Leave implementation to userland

This is a perfectly valid way to go. It does leave a lot of complexity to userland though and future updates to React & Redux might very well break implementations unintentionally. Using it directly is often a footgun, so should be discouraged.

Add escape hatch for tearing - As proposed in #1126

This was a proposal to opt-in to the old behaviour (components fetching directly from store) for first render via a prop on <Provider>, I feel this is the wrong way to go since it doesn’t tackle the problem directly and is probably a footgun. Was abandoned in #1126.

Add same functionality directly to existing connect-HOC - Add a new option to connect

Probably a viable alternative. Kind of a connect and make sure these extra reducers are present. Could either update the storeState on context so all following connects don't have to add it explicitly, or require every connect that depends on these reducers to add them explicitly. This would only apply to state, either way the reducers would be added to the store on the first connect.

Different API from the proposed one - Based around same solution

If you have better ideas for how the public API for this could look like, I’d love it! The proposed solution is a rough one to get discussions going.

Something else? - There might very well be solutions I have not thought about.

---

This is by no means a finished or polished suggestion, I’d love to get a discussion going to improve it! Would it solve your usecase? It is a good fit to include in react-redux? Etc..

I’d be happy to submit this in PR-form instead if that would be helpful, but wanted to start as an issue to encourage discussion.

[markerikson]

Thanks for filing this! We'll need some time to look this over and think about it, but I definitely appreciate you taking the time to put together a well-thought-out writeup.

First question that comes to my mind: what would a non-HOC solution look like here, especially in a hooks-based world?

Tagging @abettadapur and @mpeyper to get their thoughts, since I know both of them have done excellent work around Redux code splitting already.

[Ephem]

Of course! I think this would need a lot of time and testing to get right both design- and implementation-wise. I know a lot of things are in flux (pun intended) and uncertain right now so might not be the best of times to design and commit to maintaining a new public API but wanted to get the discussion going. :)

This proposal doesn’t necessarily contain a HOC at all, the basis is just a component. A HOC can be easily added on top of that. It’s a weird case where the WithReducer-wrapper doesn’t pass any props to its child so no HOC or render prop-design is strictly needed (but might be helpful).

I’ve been thinking about hooks too but didn’t want to complicate matters in the first draft. :) The thing to note about hooks is that since a hook can’t Provide context, a hook-only solution would have to return a component that the user wraps a subtree in manually. Definitely doable.

[Ephem]

A thing to note about the current design is that I explicitly tried to stick to what’s already in React and also make it possible to backport to <6 and legacy context if desireable. It’s probably not, but anyway.. 🤷‍♂️

[vincentjames501]

Could also add other things, such as optionally removing reducers/state on unmount.

I would love to see first class support for something like this personally. We have a lot of "feature" components tied to certain routes and when the user navigates away, it would frankly be nice to just clean this up vs having a bunch of clear state/cleanup like actions.

[markerikson]

I suppose a related question is, how much of this should be handled at the Redux store level, vs the React-Redux level?

[Ephem]

That is a good question.

I think Redux replaceReducer gives us the low level primitive we need and that most of the challenges discussed here are related to React specifically (and possibly other frameworks), such as the future concurrency model and not wanting to encourage using ReactReduxContext or store directly to avoid footguns.

If the goal and scope is broadened to provide a better/simpler high level story around code splitting/dynamic reducer injection in general, I think there is an opportunity to add functionality directly to Redux as well. That goal would basically be to remove some of the ceremony documented in the code splitting recipe I guess?

In terms of this RFC, I think what such a change to Redux would mean is that there would be a default implementation of getRootReducer that could be used out of the box, but I think most other parts would still have to be implemented in react-redux?

Either way, I do think providing low level primitives is important to retain the flexibility Redux is known for. 😄

This is all just my quite limited view on it though, did you have any specific thoughts in mind?

[Ephem]

I updated the RFC above with an extra alternative solution:

Add same functionality directly to existing connect-HOC

[mpeyper]

Conceptually, I find this very interesting as this is essentially what redux-dynamic-reducer (now deprecated) and it's successor, redux-dynostore set out to provide.

From an implementation perspective, I think there are a few things you may want to consider, especially if you want to shoot for improving the overall dynamic reducer story.

Take this snippet from the sandbox, for example:

let { reducers } = App;

function getRootReducer(newReducers) {
  reducers = { ...reducers, ...newReducers };
  return combineReducers(reducers);
}

const store = createStore(combineReducers(reducers));

There's quite a bit going on here, and a lot for someone coming into this project to need to understand:

• Why do I need to combine reducers twice?
• What happens if 2 components have reducers using the same keys? (It could just be a large app with multiple people working on it with lots of components and reducers being thrown around, or a component from a third party library that user is unaware of using WithReducers internally)
• How does hot module replacement factor into this?
• etc.

Now, arguably, some of these are easy to answer, but the point is that they need answering, and it may be setup or handled differently between different projects, which adds cognitive overhead for any devs diving into the codebase.

FWIW, redux-dynostore works around some of this by removing the how reducers get added to the store from the user's control all together. The theory is that the result of the static reducer (passed to createStore) just returns some state, so there is nothing (except this annoying warning from combineReducers) preventing you from merging that into the state returned from a different reducer (constructed from the dynamic reducers).

An advantage of your proposed version over this is it allows users to modify how the reducers are stored and merged, allowing for more customisation (e.g. immutablejs support), which is something we are currently working on in redux-dynostore. I could see libraries popping up (maybe even redux-dynostore) that are just opinionated getRootReducer functions.

Another thing I noticed in WithReducer:

    constructor(props, context) {
      super(props, context);
      const rootReducer = context.getRootReducer(props.reducers);
      context.store.replaceReducer(rootReducer);
      const firstRenderState = rootReducer(context.storeState, {}); // <----- this line
      this.firstRenderContextValue = {
        ...context,
        storeState: firstRenderState
      };
    }

Is there any reason that line could not just be const firstRenderState = context.store.getState();? The reduction has already occurred as part of replaceReducer, plus it would give any store enhancers that do something with getState (not a very common case, but it does exist) to get a crack.

Finally, I'd caution you about pushing the logic to the root of the app. In my experience, which is admittedly a bit niche in the redux space, you will invariably need some context from where the injection occurred and it's gone by the time getRootReducer sees it. That said, redux is generally considered to be a global part of the application, so it's might be hard to avoid (most of my OSS libraries exist to fight against this assumption).

Now, my 2 cents on your questions:

• Naming (Is getRootReducer really a good name considering it’s basically expected to have sideeffects? Should WithReducer be called something else? Etc..)

I agree that the getRootReducer name is not great. I don't have a solid alternative, but in redux-dynostore we use create... in these situations.

WithReducers is fine 😃.

• When we have a component like this, it’s easy to add a HOC based on it, should this be part of the official implementation? Component version leaves forwardRef and hoisting statics to userland, HOC could do this behind the scenes.

If every single WithReducers wrapper looks the same, i.e.

export default props => (
  <WithReducers reducers={redcuers}>
    <ConnectedComponent {...props} />
  </WithReducers>
);

Then this is a good use case for a HOC. I'm personally a fan of including the functionality in the connect HOC as I see these being used almost exclusively in tandem with each other, and introducing a HOC to mount a component that inserts a provider around a connected component is I think exactly the sort of cascading components that the react hooks proposal is trying to avoid. Essentially, the component tree will end up something like:

• withReducers
  • WithReducers (assuming you want to support the non-HOC version too)
    • ReactReduxContext.Provider
      • connect
        • Component

By including it in the connect HOC, you likely won't need to introduce any additional layers, but you do lose the non-HOC version (until redux inevitably does something with hooks).

• This solution does not take into account other sideeffects such as adding sagas etc. Might be possible to add those in userland in getRootReducer since you often have access to store there, but then it should be renamed. Not sure if it’s in scope for this RFC.

TBH, this is the main reason why I think this whole thing is best kept in userland. The redux ecosystem is huge, varied and ever-changing. You will never support all of it, especially when targeting the dynamic use cases.

You could reasonably say that reducers are the only required part of redux so dynamic reducers the only one that is supported out-of-the-box (to which I would counter argument that it already is by replaceReducers 😉), and all the other libraries will need to support dynamic behaviour themselves.

• Optionally removing reducers on unmount?

Yes. This was one of the first questions/requests we always got with redux-dynostore when people first came across it and was recently implemented. It wasn't difficult to do and supports more use cases.

What would I do?

Make ReactReduxContext part of public API

This. It's part of the private API already, but so widely used (at least the legacy version), that it's going to be very difficult to ignore it moving forward anyway.

If you really must hide it, then a StoreInterceptor component would be a useful addition to react-redux. It would push the store into a render prop and let the user have at it. Then WithReducer could be implemented more easily in userland. Here is a sandbox demonstrating the idea (forked from yours).

Oh man, I went overboard with that sandbox, but I do really enjoy this problem! The actual semantics of how StoreInterceptor should work and when the store interceptor fires (e.g. in constructor only, on updates, every render, etc.) are all up for debate.

In the process of setting that sandbox up, I discovered that you cannot call replaceReducer inside the render function as it ends up calling setState in the Provider component and React yells at you. This may make implementing anything like this in a hook enabled world a little more difficult.

Also, I know I mentioned about about avoiding extra component layers and I'm aware this implementation does not fare well in that regard either. Don't judge me too harshly.

I hope some of this helps, and I'm always happy to discuss this more.

[Ephem]

@mpeyper Wow, that's a deep and thoughtful comment, thank you so much for writing it! 💯

Edit: This turned out to become a lot longer than I intended, sorry for the wall of text. Turns out this is really hard to write about concisely.. 😅

Conceptually, I find this very interesting as this is essentially what redux-dynamic-reducer (now deprecated) and it's successor, redux-dynostore set out to provide.

This RFC was heavily influenced and motivated by prior work like that, I've looked at redux-dynostore specifically and like it, thanks for all your work on it.

Is there any reason that line could not just be const firstRenderState = context.store.getState();?

This is at the heart of the problem. In a concurrent world, another action might have fired between the start of rendering when storeState was put on the context and the time of reducer-injection, I think. Updating the state for a subtree via store.getState() would include the new state from that action, while other parts of the component tree would still see the old state, leading to tearing. The complexity of getting concurrency and SSR right is what motivated me to write this RFC.

High level vs low level, opinionated vs unopinionated

Just to be clear, this RFC mainly aim to be low level and unopinionated, providing a better integration-point to build solutions in userland on top of.

TBH, this is the main reason why I think this whole thing is best kept in userland. The redux ecosystem is huge, varied and ever-changing. You will never support all of it, especially when targeting the dynamic use cases.
You could reasonably say that reducers are the only required part of redux so dynamic reducers the only one that is supported out-of-the-box (to which I would counter argument that it already is by replaceReducers 😉), and all the other libraries will need to support dynamic behaviour themselves.

I agree with almost everything you are saying here (and you said it well), except one thing. While I do think that replaceReducer is a good enough low-level API for Redux itself, I think it's no longer quite enough when working with React because of the complexities mentioned above. Left to userland, many libraries will get concurrency and/or SSR wrong as I definitely would, and probably have in this RFC/demo-implementation too. 😃

If we can simultaneously improve the built-in experience for simple cases, that would be a great win, but is not the main point of this RFC to be honest.

An advantage of your proposed version over this is it allows users to modify how the reducers are stored and merged, allowing for more customisation (e.g. immutablejs support), which is something we are currently working on in redux-dynostore. I could see libraries popping up (maybe even redux-dynostore) that are just opinionated getRootReducer functions.

This was exactly the vision that I had in mind when writing the RFC, I explicitly tried ot make it so low level and unopinionated that libraries like redux-dynostore could build their opinionated solutions on top. 😄

Finally, I'd caution you about pushing the logic to the root of the app.

I haven't had time to dig into this very deeply so I might very well be wrong, but I suspect this related to why you are having problems with SSR in redux-dynostore. Any ergonomic solution I can think of when it comes to dynamic reducer injection needs to be stateful (keep track of which reducers has been injected). In SSR, this state needs to be kept per request (just like the Redux-store) and can not be global which is why this part of the solution needs to be lifted to the top and sent down on the context. Each request needs its own reducer registry so to speak.

I definitely agree that you might sometimes need some more of the context of the call site though, definitely room for improvement in the RFC there. 👍

Feedback on RFC

Thank you for the specific feedback on the RFC, I agree with most of your comments. :) HOC should be included, remove on unmount as well.

createRootReducer is a lot better than getRootReducer, but there might still be room for improvement. While this function needs to return a root-reducer, since it can have any number of sideeffects really, I'm not even sure it should be called anything related to reducer. On a related note, it can take basically any parameters, so the reducer-prop of <WithReducers> that gets sent to that function maybe shouldn't be called reducers either? Or maybe it should just support multiple props? I think there is a lot of room for improvement here, especially related to supporting more dynamic use-cases like sagas.

Maybe: <WithReducers reducers={} options={}> and something like function inject(reducers, options) {} or something more along those lines? Since you often have access to store where the inject-function is declared, maybe this would be enough to support more complex use-cases like sagas at least a little better in userland?

Even though this won't be as common as using connect, I also agree with trying to minimize the component cascade as much as possible. I'm not sure about including the functionality in connect though just from a separation of concerns perspective. I don't have any concrete arguments there at the moment though and don't feel very strongly about it.

What would I do & StoreInterceptor

I agree that accessing the store from context is not going away anytime soon, if ever, even if it's unofficial. ReactReduxContext is exported in v6 for this reason. I think there are at least a couple of reasons to discourage its use and keep it a non-public API:

1. As we've seen, it is easy to create unintentional bugs when using it directly
2. Some specific (current or future) userland-solutions that build directly on top of store might make it harder to change internals of Redux/React-Redux in the future(?)
3. React-Redux are the official React-bindings for Redux, if we need to access store directly, this might be a sign there is room to improve the React-Redux API, like the one we are discussing

I'm pretty sure I've seen @markerikson write about/discuss this several times in different places, maybe you could fill in what I have missed or link to previous discussions?

When I started thinking about this RFC I also tried to come up with ways to have an even more generic low-level API, but that still avoided most footguns. In my head <WithReducers> is actually conceptually still called <ReduxContextUpdater>, a component that is designed to patch the Redux context safely for any use case. That turned out to be too generic though.

Oh man, I went overboard with that sandbox, but I do really enjoy this problem!

That makes two of us! 😉

I really like your implementation when considering a client-side application in the current state of React. When considering concurrency and SSR I see a couple of issues though:

1. The reducerRegistry is a singleton which would not work with SSR
2. <Provider> is not meant to be used several times(?), using it here sets up a new subscription to the Redux-store when there is only meant to be one, not sure about the consequences
3. Since <Provider> calls store.getState() behind the scenes, this might cause tearing with concurrent rendering
4. (<Provider> is kept in the component tree indefinitely when it is really only needed for that first render when the reducer injection happens)

This is suuuper-hard and has tripped me up soo many times already, I'm pretty certain I still have similar bugs in my demo-implementation and even if not, before concurrent rendering is finished and final, I'm certain I will.

These complexities is exactly why I feel there is a need for a better integration-point in React-Redux for adding reducers inside of the component tree (that is, provide an easy way to patch the ReactReduxContext safely in these cases).

A downside to this is of course that it would lead to an increased maintainer-burden in this repo instead of in userland and any bugs would probably have an even higher impact in the community. On the other hand they would hopefully get fixed faster and only have to be fixed once. If any problems arise directly from the design of such a solution and not its implementation, that would be really bad though, which is why this RFC was so scary to write..

I'm far from certain my RFC tackles this the best way, my only hope is that it describes the problem-space decently.

I hope some of this helps

It really did! I'll incorporate a bunch of this next time I revise the RFC! ⭐️

[Ephem]

@mpeyper If you could dream, what would a ergonomic and flexible API to implement redux-dynostore on top of look like? That let you not worry about these intricacies that is.

How much of the current redux-dynostore could be implemented on top of this RFC? What is not covered?

[abettadapur]

I author redux-dynamic-modules, which depends on having direct access to the store in some fashion.

We export a createStore function from our library, which creates a Redux store, and also extends the object with the addModule(s) functions. The addModule function takes one of our 'modules', which can contain reducers, middleware, sagas, etc, and adds ALL of its contents to the store/saga middleware.

We then have a higher order component DynamicModuleLoader which accesses the store from the context and adds whatever modules it needs using addModule.

Because 'modules' can contain any kind of redux artifacts (reducers, middleware, epics, sagas), a single WithReducers component would not really work so well for our library. That's not to say it wouldn't work for simpler scenarios, but I think that libraries that try to solve more complicated problems will always need some kind of direct access to the store.

[Ephem]

Great comment @abettadapur, thank you, and thank you for your work on redux-dynamic-modules!

I'm definitely not proposing we remove access to store or the context, only that we add a safer way to do so for the specific use-case of adding reducers. I agree that there will probably always be complex cases that needs access to store directly.

In this RFC you might choose to do other things to store in the currently badly named getRootReducer, where you probably already have access to it, that way you don't have to fetch it from context, but you still could if you wanted to. Libraries would also be encouraged to wrap the <WithReducers>-component inside their own HOCs/components if it made sense, to take care of the reducer-injection part.

I have not yet thought much about what sideeffects dynamically adding middlewares, epics or sagas could have in a concurrent world and also don't have any experience in adding them dynamically. Not adding specific support for them in the RFC stems both from wanting to keep scope down, and inexperience and lack of knowledge on my part, suggestions in this area are very welcome!

In what phase are these typically injected? In a concurrent world I guess it would make sense to inject them in the commit-phase (since they are probably not needed until an action is fired which I guess would happen at a later point)? If they were injected in the render-phase, this could lead to actions from a different fiber going through middleware/sagas/epics that were added in a work-in-progress fiber that has not yet been committed? Maybe this is not a huge problem?

The tricky part about reducers specifically is that they need to be injected before rendering and the state change from this be made available outside of the store. If other things needs to be added after rendering, maybe there would be room to rename and expand <WithReducers> to include separate injection-points before and after? Not sure how this would work with SSR though.

@abettadapur @mpeyper In your eyes, how could this RFC be improved to better support dynamically injecting other things than reducers?

[mpeyper]

@Ephem Thanks for the detailed reply. All your points make sense from an SSR perspective and wanting to support the common case. Your point on why context.store.getState() cannot be used in WithReducers was particularly enlightening. One bit that I'm not too clear on is

1. The reducerRegistry is a singleton which would not work with SSR

I'm not sure how the reducerRegistry is any more a singleton than the reducers variable in your solution. I could be missing something, and javascript is not my native language, and SSR is completely foreign to me, so I'm willing to accept that perhaps I just don't know enough to understand this yet.

I have often admitted that what I work on (and how IOOF in uses redux in general) is a bit left of normal and redux and react-redux would likely be worse and overcomplicated if it tried to support it directly.

That said:

@mpeyper If you could dream, what would a ergonomic and flexible API to implement redux-dynostore on top of look like? That let you not worry about these intricacies that is.

How much of the current redux-dynostore could be implemented on top of this RFC? What is not covered?

redux-dynostore has very lofty goals of enable dynamic anything by essentially providing a store enhancer adds store enhancers (albeit it a slightly more controlled way) and a HOC that calls other HOCs (albeit in a slightly more controlled way) to call those enhancers, i.e. the dynostore enhancer and dynamic HOC without any of the enhancers has a zero effect on anything. Most (all?) of the enhancers abuse the store in ways that are probably not recommended and rely on there being a single global store (they generally append functions to the store in one way or another).

It sounds more complicated than it actually is, but the point is that this RFC would assist a single store enhancer (dynamicReducers) and a single component enhancer (attachReducers), assuming there is no good way to incorporate the larger ecosystem, in which case the whole lot might be replaceable.

To be clear, if I could replace all my libraries with build in redux/react-redux functionality, I would.

It's a bit hard to say what my dream API would look like because if I knew that I would have raised an issue or a PR a long time ago, but I think replaceReducer, while incredibly flexible, pushes a lot of the complexity into the libraries for managing everything else. If the internals of redux were modified to support the concept of having zero or more root reducers that had their results (shallowly?) merged into a single state tree and there was an addReducer and removeReducer functions that allowed modifying the root reducers, this would make dynamic reducers much easier. This does nothing for the rest of the ecosystem though.

@abettadapur @mpeyper In your eyes, how could this RFC be improved to better support dynamically injecting other things than reducers?

I don't have any suggestion for improvements, just observations from me experiences dealing with the redux ecosystem and strange use cases.

In general, anything other that reducers is going to concern itself with either generating or listening to actions. My knowledge of SSR is admittedly a bit weak, but I think this means that dynamically adding them in an SSR scenario does not make much sense or at least, as you suggested, would have no effect on the returned output.

If we restrict the conversation to the "big ones", redux-saga and redux-observable (most others will either work similarly to these, gain nothing from being dynamic, or don't currently support dynamic changes in any way), they both support dynamically adding sagas/epics by calling the run` function with the dynamic item on their respective middlewares.

Looking passed the naming issues, this means that your suggestion of using the getRootReducer function to support them should be possible, if you could identify them somehow (perhaps a simple as a naming convention?):

let { reducers, sagas, epics } = App;

const sagaMiddleware = createSagaMiddleware();
const epicMiddleware = createEpicMiddleware();

function getRootReducer(newProps) {
  reducers = { ...reducers, ...newProps.reducers };
  newProps.sagas.forEach(sagaMiddleware.run);
  newProps.epics.forEach(epicMiddleware.run);
  return combineReducers(reducers);
}

const store = createStore(combineReducers(reducers), applyMiddleware(sagaMiddleware, epicMiddleware));

sagas.forEach(sagaMiddleware.run);
epics.forEach(epicMiddleware.run);

export default props => (
    <WithReducers reducers={{ reducers: newReducers, sagas: newSagas, epics: newEpics}}>
        <ConnectedGreeter {...props} />
    </WithReducers>
);

One issue this has is that these middleware don't care if if you call them with the same saga or epic multiple times, so some care may need to be taken to prevent this on mounting multiple instances conditionally rendered components. If sagas/epics were to be objects with keys instead of arrays this could looks something like:

let { reducers, sagas, epics } = App;

const sagaMiddleware = createSagaMiddleware()
const epicMiddleware = createEpicMiddleware()

const runningSagas = Object.keys(sagas)
const runningEpics = Object.keys(epics)

function getRootReducer(newProps) {
  reducers = { ...reducers, ...newProps.reducers };
  const newSagaKeys = Object.keys(newProps.sagas).filter(key => !runningSagas.includes(key))
  const newEpicKeys = Object.keys(newProps.epics).filter(key => !runningEpics.includes(key))
  
  newSagaKeys.forEach(key => sagaMiddleware.run(newProps.sagas[key]);
  newEpicKeys.forEach(key => epicMiddleware.run(newProps.epics[key]);

  return combineReducers(reducers);
}

const store = createStore(combineReducers(reducers), applyMiddleware(sagaMiddleware, epicMiddleware));

sagas.forEach(sagaMiddleware.run)
epics.forEach(epicMiddleware.run)

There are likely issues with this as well, such as running the sagas/epics before the replaceReducers call has been made, which will need to worked through. I think having seperate injection-points for before and after may resolve most of the issues that may arise, but that's just a guess.

My biggest concern is with each middleware the complexity and understanding required of what getRootReducer is doing gets bigger and bigger and the chance of someone introducing a hard to find (trust me) bug into their app increases. I will concede that most users pick one async/side-effect middleware and run with it, rather than mixing and matching for different use cases, so it's likely the messiness will be relatively contained.

I'm also not sure how this would look if we start thinking about library authors providing opinionated variations of getRootReducer to handle different parts of the newProps, where only one of them is expected to actually return a reducer.

Edit: This turned out to become a lot longer than I intended, sorry for the wall of text. Turns out this is really hard to write about concisely.. 😅

Yep... I've got this problem too 😛

[Ephem]

I'm not sure how the reducerRegistry is any more a singleton than the reducers variable in your solution. I could be missing something, and javascript is not my native language, and SSR is completely foreign to me, so I'm willing to accept that perhaps I just don't know enough to understand this yet.

It was not, so sorry for confusing you.. :( I just realized I never implemented the wrapper I was going to, my sandbox is updated.

The main difference between the solutions is that since yours is based on importing the reducerRegistry from anywhere in the app, there can really only be one. You could easily wrap yours in a similar wrapper, say createReducerRegistry that each had a unique reducers-variable, but you would need someplace to store and access that resulting reducerRegistry and the context seems like the logical place.

A simplified SSR usage-example for that, borrowing from your solution, could look like this:

// Express endpoint
app.get('/', (req, res) => {
    const store = createStore(topReducer);
    const reducerRegistry = createReducerRegistry();
    const html = ReactDOMServer.render(
        <Provider store={store}>
            <ReducerRegistryContext.Provider value={reducerRegistry}>
                <App />
            </ReducerRegistryContext.Provider>
        </Provider>
    );
    res.render('index', { html });
});

Then you would pick up reducerRegistry deeper in the app via context. The ability to create that unique reducerRegistry per request on the server side is the reason that specific piece of logic needs to be hoisted to the top unfortunately.

The rest of your comment around sagas and epics was illuminating, I'll mull it over, research some more and get back with a more detailed comment on that topic.

Just to clarify though, the getRootReducer from my example was just to show the simplest possible implementation in userland, that part is not part of the RFC at all more than showing the API, the user is supposed to implement that themselves. They could do this either by following a simple recipe from the docs, or importing a ready-made solution from a library they like.

[Ephem]

I'm playing with the idea that maybe getRootReducer could be called modifyStore or storeModifier instead?

const reducers = { ...staticReducers };
function modifyStore(replaceReducer, options) {
    reducers = { ...reducers, ...options.reducers };

    // Put stuff to modify the store before replacing reducers here

    replaceReducer(combineReducers(reducers));

    // Put stuff to modify the store after replacing reducers here
}

ReactDOM.render(
    <Provider store={store} storeModifier={modifyStore}>
        <App />
    </Provider>
);

The replaceReducer passed into this function wraps the store.replaceReducer-one in order to extract the root reducer so we can patch the storeState on context properly from the component.

<WithReducers reducers={}> could be renamed <WithModifiedStore options={}> or something similar.

Variant

Pass store directly into the function, with a spy attached to replaceReducer. If replaceReducer was called in modifyStore, take care of safely patching the storeState in the <WithModifiedStore>-component.

---

Either approach should make it a bit more flexible to do other stuff than just replacing the root reducer, and also make it clear that this is intended. The storeModifier itself should be implemented in userland and would be the integration point for libraries. Will most likely not solve every complex usecase out there, but maybe more than the original RFC?

BTW, the only guarantee React-Redux makes about the storeModifier is that it safely patches storeState in case you replace the rootReducer in there. If you do other wonky things in userland, you are on your own. 😄

I haven't thought this through, just tossing the idea out there for early feedback. 😄

[mpeyper]

I like the look of this new approach.

With a little bit of work, the store modifier could be made into a modifier chain (similar to redux's middleware chain) that would allow more modular store modifications to occur. I think passing through the whole store (or a storeAPI?) would make the chain concept a bit nicer too (especially for libraries that enhance the store with additional functions, such as redux-dynostore and redux-dynamic-modules).

Setting it up might look something like (stolen heavily from applyMiddleware):

let modifyStore = () => {
  throw new Error('not yet!');
}

const chain = modifiers.map(modifier => modifier({ ...store, modifyStore }));
modifyStore = compose(...chain)(() => {}); // `modifyStore` without any modifiers does nothing.

Then you would invoke it using the options provided from WithModifiedStore:

modifyStore(options);

Now we can split the modifiers into nice pieces, and even have modifiers call other modifiers. e.g:

const reducers = { ...staticReducers };
const dynamicReducers = store => next => options => {
  reducers = { ...reducers, ...options.reducers };
  store.replaceReducer(combineReducers(reducers));
  next(options);
}

const sagas = { ...staticSagas };
const dynamicSagas = (sagaMiddleware) => () => next => options => {
  const newSagaKeys = Object.keys(options.sagas).filter(key => ! sagas.includes(key));
  
  Object.keys(options.sagas)
    .filter(key => !runningSagas.includes(key))
    .forEach(key => {
      const saga = options.sagas[key];
      sagaMiddleware.run(saga);
      sagas[key] = saga;
    });

  next(options);
}

const dynamicModules = ({ addModules }) => next => options => {
  addModules(options.modules); // I have no idea if redux-dynamic-module handles adding the same module multiple times, but this is just for demonstration purposes
  next(options);
}

// dumb example
const conditionalReducers = (predicate, reducers) => store => next => options => {
  if (predicate(store)) {
    store.modifyStore({ reducers });
  }
  next(options);
}

const storeModifiers = [
  dynamicReducers,
  dynamicSagas(sagaMiddleware),
  dynamicModules,
  conditionalReducers(({ getState }) => getState().user.isAdmin, { adminSettings })
];

ReactDOM.render(
  <Provider store={store} storeModifiers={storeModifiers}>
    <App />
  </Provider>
);

Like middleware, the order in which the are provided would determine the order they are invoked (e.g. to ensure the reducers get added before the sagas are run). I've probably got the order wrong above, but 🤷‍♂️.

NOTE: this was all written in github editor and not tested. I'm always a little iffy when trying to use things like compose so there may need to be some tweeks and before it works.

---

I find the WithModifiedStore (and With... as a component name in general) a bit jarring as the thing it's providing the modified store to might be a deeply nested child or may not even exist at all if it's just running a saga. I think a small change to ModifyStore would be better (open to other ideas too. Making it an option in the connect HOC also fixes this, but is less flexible than having an intermediate component, which I'm coming around to as being the better idea as otherwise we may start to see components being connected just to add store modifications (i.e. not mapping any state or dispatches to use).

---

EDIT

I just realised that this suggestion is starting to look a lot like redux-dynostore's enhancers. I could envision @redux-dynostore/core (the thing that adds all the extra functions to the store) not changing at all and @redux-dynostore/react-redux exporting a bunch of modifiers to use with Provider and dynamic just being a thin wrapper around the WithModifiedStore component, ensuring the provided things went into the correct keys of the options for the modifier to pick up.

[Ephem]

I totally agree with you that With... as a component name is jarring, I never liked it and added it mostly for clarity in the RFC around what the component is supposed to do. Now that we've come this far, I would suggest that the top function would be called modifyStore (verb), the component be called StoreModifier (noun) and the HOC withModifiedStore.

I really like the idea of a chain! Only problem that would need to be solved is clashing options I guess. What happens if you add several modifiers that unknowingly uses the same option-keys? I'm sure there would be a number of ways to solve that by namespacing though.

I also agree that passing the store, or a (possibly stripped down?) StoreAPI is the better option.

That EDIT is exactly where I was hoping to bring this RFC! Making it possible and hopefully easier to implement both existing and new solutions on top of.

I'm starting to have a good idea what an updated RFC would look like, but I don't want to edit the original comment since that would cause confusion for new readers. I think at this point (after initial feedback) it makes sense to instead open a PR with the updated RFC (and possibly an example implementation) to help further discussion and so we can track changes.

I might leave the chain out of the original PR just to keep it simple at first, in that case with the intention of adding it. I'll write it up ASAP.

Thank you so much for your comments @mpeyper and @abettadapur! Do you have any thoughts on this new approach @abettadapur?

[abettadapur]

@Ephem @mpeyper This is essentially what we do with redux-dynamic-modules, we have an 'extension' concept where anyone can create an extension that takes a module and modifies the store in a different way.

For example, the saga extension (in typescript):

/**
 * Get an extension that integrates saga with the store
 * @param sagaContext The context to provide to the saga
 */
export function getSagaExtension<C>(sagaContext?: C, onError?: (error: Error) => void): IExtension {
    let sagaMonitor = undefined;

    //@ts-ignore
    if (process.env.NODE_ENV === "development") {
        sagaMonitor = window["__SAGA_MONITOR_EXTENSION__"] || undefined;
    }

    // Setup the saga middleware
    let sagaMiddleware: SagaMiddleware<C> = createSagaMiddleware<any>({
        context: sagaContext,
        sagaMonitor,
        onError,
    });

    let _sagaManager: IItemManager<ISagaRegistration<any>> = getRefCountedManager(getSagaManager(sagaMiddleware), sagaEquals);

    return {
        middleware: [sagaMiddleware],

        // perform initialization here
        onModuleManagerCreated: (moduleManager: IModuleManager) => {
            if (sagaContext) {
                sagaContext["moduleManager"] = moduleManager;
            }
        },

       // perform behavior when module is added
        onModuleAdded: (module: ISagaModule<any>): void => {
            if (module.sagas) {
                _sagaManager.add(module.sagas);
            }
        },

        // perform behavior when module is removed
        onModuleRemoved: (module: ISagaModule<any>): void => {
            if (module.sagas) {
                _sagaManager.remove(module.sagas);
            }
        },

        dispose: () => {
            _sagaManager.dispose();
        },
    };
}

I think the built in concept of a 'store modifier' is a great idea. Libraries like saga/observable can then leverage the API and publish their own 'modifiers', making it easy for users to consume this concept.

Maybe I missed it in the text above, but one question: where is the 'modifier' chain called from? The <WithReducers/> HOC? How does the saga modifier get the sagas it needs to add, for instance?

[Ephem]

Thats a great example, thank you! I should probably take a closer look at the src for redux-dynamic-modules..

I've rewritten the RFC and is currently working on a demo implementation, so hopefully I'll have a PR up soon. I think that RFC is a bit clearer on what you are asking and with some (desperately needed) better naming as well. Having a PR will hopefully also make it easier to discuss different details.

In this current RFC, it is the options in <WithReducers options={options}> that gets passed along to the modifier(-chain), so that's the place you define which sagas you want to have added.

In the new RFC this is currently: <StoreModifier modification={}> or withModifiedStore(modification). The idea here is that a "modification" is basically the same concept as an action, it's simply an object that describes a desired change to the store. Not a 100% convinced about that name either, but naming things is hard.. ^^

[Ephem]

I'm going to go ahead and close this and the related PR, as there are a lot of more important things to tackle short-term (as per this roadmap). While I still think a public API for injecting reducers from the component tree is needed, we should give other things some time to stabilise before revisiting the details around this.

The roadmap suggests contributing tests for these cases, and while they will necessarily test private APIs and therefor be somewhat brittle, I agree that this is the best way forward right now.