-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
RFC - StoreModifier, dynamically modifying store from component tree #1154
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,185 @@ | ||
# RFC - Dynamically modify the store from within the component tree | ||
|
||
The original version of this RFC can be found in [#1150](https://github.com/reduxjs/react-redux/issues/1150) | ||
|
||
The goal of this RFC is to provide an official way to modify the store from within the component tree (without getting it directly from `ReactReduxContext`). Modifications to the store might result in subtle bugs, especially with concurrent rendering, and should be considered unsafe. | ||
|
||
The only guarantee React-Redux makes, is that if you use `replaceReducer` as a modification, the `storeState` on context will be patched with the new state for all children, even for the current render. | ||
|
||
The secondary goal of this RFC is to make it easier out of the box to code split reducers. | ||
|
||
(This file itself is never meant to be merged) | ||
|
||
## Summary of problem (recap of #1126) | ||
|
||
Omitted for brevity, see summary of the problem space in [#1150](https://github.com/reduxjs/react-redux/issues/1150), read early discussions in [#1126](https://github.com/reduxjs/react-redux/issues/1126) | ||
|
||
## 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. I've listed alternative solutions towards the end. | ||
|
||
My hope is that this solution strikes a balance between being immediately useful and preserving the flexibility for users/libraries to construct a root-reducer and injection techniques however they feel like. | ||
|
||
I am NOT proposing to hide the `ReactReduxContext` or disallow getting the store directly from it, merely to keep it as a non-public API for usecases not covered by this RFC. | ||
|
||
**Goals:** | ||
|
||
- Add API for adding reducers from the component tree | ||
- Optionally: Make it easier/safer to do other dynamic store modifications (register sagas, epics etc) | ||
- Do not require direct use of `store` or `ReactReduxContext` in userland | ||
- Flexible API that supports both simple and complex usecases well | ||
- Both existing and new libraries should be able to build more complex solutions on top of this API, but not have to worry about the intricacies of patching `storeState` on context when replacing reducers | ||
- Small surface area | ||
- Aligned with `react-redux` v6 approach for preparing for concurrent-safeness | ||
- SSR-support | ||
|
||
### Example usage | ||
|
||
(Some imports etc omitted for brevity) | ||
|
||
```jsx | ||
import { withModifiedStore } from 'react-redux' | ||
import reducer from './namesReducer' | ||
|
||
function Greeter({ currentName }) { | ||
return `Hi ${currentName}!` | ||
} | ||
|
||
const mapStateToProps = state => ({ | ||
currentName: state.names.currentName | ||
}) | ||
|
||
// Structure of this object is not prescriptive, | ||
// it depends entirely on the implementation of | ||
// modifyStore | ||
// In a sense this is quite like an action, in that it | ||
// is an object that should describe the store modification | ||
const modification = { | ||
newReducers: { | ||
names: reducer | ||
} | ||
} | ||
|
||
export default withModifiedStore(modification)( | ||
connect(mapStateToProps)(Greeter) | ||
) | ||
``` | ||
|
||
```jsx | ||
import staticReducers from './staticReducers' | ||
|
||
// Example simple userland implementation | ||
let reducers = { ...staticReducers } | ||
function modifyStore({ newReducers }, storeProxy, state) { | ||
reducers = { ...reducers, ...newReducers } | ||
storeProxy.replaceReducer(combineReducers(reducers)) | ||
} | ||
|
||
const DynamicGreeter = React.lazy(() => import('./Greeter')) | ||
|
||
ReactDOM.render( | ||
<Provider store={store} modifyStore={modifyStore}> | ||
<Suspense fallback={<div>Loading...</div>}> | ||
<DynamicGreeter /> | ||
</Suspense> | ||
</Provider> | ||
) | ||
``` | ||
|
||
### API | ||
|
||
#### `modifyStore(modification, storeProxy, state)` | ||
|
||
This is a callback that is supposed to be implemented by the user/by libraries and passed into `Provider` or directly into `StoreModifier` or `withModifiedStore`. | ||
|
||
Most implementations of `modifyStore` need to be stateful, and the only way to support this for SSR is to create one stateful instance per request. Passing it into `<Provider>` puts the function on the context behind the scenes and is the recommended approach even if SSR is not needed. | ||
|
||
**Arguments (automatically passed in to the function)** | ||
|
||
1. `modification` is an object that can contain any data that needs to be passed from the calling site (`<StoreModifier>`), for example reducers to be added to the store. There is no prescribed shape for this object, it is supposed to differ between implementations | ||
|
||
2. `storeProxy` here is a wrapped version of `store` that spies on `replaceReducer` to be able to safely patch the context with the new state if it is called. Maybe this should be a stripped down version of `store`, leaving out `subscribe` and possibly other methods unsafe to use here? Could also keep them in but provide warnings. | ||
|
||
3. `state` is passed in, in case it is needed for conditional logic, because `getState` is kind of unsafe here. This `state`-object contains the current state _stored on context for this render_ and is not the latest version of the state in the store. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If It could also return the updated state (that will be replaced in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I do agree it would be nice if you could get the resulting state from 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, the store has 4 functions by default:
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 |
||
|
||
**Returns** | ||
|
||
This function can optionally return a cleanup-function, this will be called when the calling component unmounts. This way users can optionally implement functionality for removing reducers/unregistering sagas/epics when they are no longer needed. | ||
|
||
#### `<StoreModifier modification={} modifyStore={}>{children}</StoreModifier>` | ||
|
||
This component calls the `modifyStore` with the `modification` provided, a `storeProxy` and the current `storeState` from context. If `modifyStore` is not passed in directly, it is read from context. This only happens once. If a callback is returned from `modifyStore`, this is called on unmount. | ||
|
||
If `replaceReducer` is called in `modifyStore`, this component wraps the children with a new context-provider with an updated `storeState`. The provider is only rendered on the first render pass since `storeState` will be correct after this. | ||
|
||
**Arguments** | ||
|
||
1. `modification` is an object that will be passed into `modifyStore` | ||
|
||
2. `modifyStore` is an optional prop that allows for passing in a `modifyStore`-function directly to `<StoreModifier>`. This makes it possible to avoid having to pass it down via context and simplifying the API when SSR-support is not needed (libraries could support both versions). | ||
|
||
3. `children` are the children that needs a modified store | ||
|
||
#### `withModifiedStore(modification, options)(WrappedComponent)` | ||
|
||
This is a Higher-Order-Component that wraps the `WrappedComponent` in a `<StoreModifier>`. It additionally takes care of hoisting statics on `WrappedComponent` and can optionally `forwardRef` to the wrapped component. As opposed to using `<StoreModifier>` directly, this HOC also supports using a custom `context` (if you are using something other than the default `ReactReduxContext`). | ||
|
||
**Arguments** | ||
|
||
1. `modification` is an object that will be passed into `modifyStore` | ||
|
||
2. `options` is an optional object with the shape of: `{ context, forwardRef, modifyStore }`. `context` can be used to consume a custom context. If `forwardRef` is true, adding a ref to the connected wrapper component will actually return the instance of the wrapped component, (both these correspond to the same options in `connect`). `modifyStore` is for passing in a custom `modifyStore`-function, see description in `<StoreModifier>`. | ||
|
||
3. `WrappedComponent` is the component to be wrapped with `<StoreModifier>` | ||
|
||
**Returns** | ||
|
||
A component that wraps the `WrappedComponent` in a `StoreModifier` | ||
|
||
#### `createStoreModifier(context)` | ||
|
||
This function returns a `StoreModifier`-component that uses the custom context provided to it. Can be used when a custom context is needed, but you don't want to use the HOC that supports this out of the box (libraries that want to avoid an extra component nesting for example). | ||
|
||
**Arguments** | ||
|
||
1. `context` is the custom context that you want the resulting component to use | ||
|
||
**Returns** | ||
|
||
A `StoreModifier`-component that uses the `context` passed in | ||
|
||
### Unsolved questions | ||
|
||
- Can naming be improved? | ||
- It would be possible to support a middleware-style API for `modifyStore`, [as per this comment](https://github.com/reduxjs/react-redux/issues/1150#issuecomment-450596584). This could be implemented in userland, but it might make sense to add this as the official API to encourage modular `storeModifiers`? | ||
- In that case: How do we deal with namespacing-issues in the `modifier`-object? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Option 1: Don't. Most things in Option 2: Perhaps if we use the proposed compose function above, we could use it to pass in slices of the options, similarly to 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm leaning towards Option 1, but with the caveat of providing advice along these lines in the documentation (better formulated..):
The It would of course be possible to rename |
||
- API? `modifyStore` can take either a function or an array of functions? Or should users call a `composeStoreModifiers` manually if they wish to use several? | ||
- Can part of the initial setup of a store also be described as a `modification` that is run manually after store creation? For example running sagas etc. | ||
- If the above two points are true, should the primary concept of `storeModifier`/`storeModifiers` actually live in Redux, while only the `<Provider>`, `<StoreModifier>` and `withModifiedStore` be implemented here in React-Redux? | ||
- Is this feasible to implement and maintain? | ||
- Is this API flexible enough to build on safely as "concurrent rendering" evolves? Are there scenarios in which this API becomes limiting and hard to support? | ||
- Should there be a default `modifyStore`-function that supports simple cases (that is, dynamically adding reducers)? What should it look like? | ||
|
||
I think it would be possible to backport this to React-Redux <6 using legacy context. The positive side of that is that any library based on this solution could then support React-Redux <6 out of the box which would also allow for an incremental upgrade-path. Considering increased maintainer burden and lessened incentives to upgrade to v6, this might not be desirable though. | ||
|
||
## Alternative solutions | ||
|
||
Details omitted for brevity, see descriptions for these alternative solutions in [#1150](https://github.com/reduxjs/react-redux/issues/1150) | ||
|
||
**Status quo** - Determine that it is fine that these solutions rely on non-public APIs | ||
|
||
**Make ReactReduxContext part of public API** - Leave implementation to userland | ||
|
||
**Add escape hatch for tearing** - As proposed in [#1126](https://github.com/reduxjs/react-redux/issues/1126) | ||
|
||
**Add same functionality directly to existing connect-HOC** - Add a new option to connect | ||
|
||
**Different API from the proposed one** - Based around same solution | ||
|
||
**Something else?** - There might very well be solutions I have not thought about. | ||
|
||
The actual implementation of this RFC could conceivably be hooks-based if backwards compatibility (to 16.4) is sacrificed, but it would be tricky to implement an ergonomic API that **is a hook** since they can not provide context, only consume it. I might have missed some smart solution though. | ||
|
||
--- | ||
|
||
This is by no means a finished 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.. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import React from 'react' | ||
import PropTypes from 'prop-types' | ||
import { ReactReduxContext } from './Context' | ||
|
||
export function createStoreModifier(Context = ReactReduxContext) { | ||
class StoreModifier extends React.Component { | ||
constructor(props, context) { | ||
super(props, context) | ||
|
||
this.firstContext = context | ||
|
||
const { store, storeState } = this.context | ||
const modifyStore = this.props.modifyStore || this.context.modifyStore | ||
|
||
// Create storeProxy | ||
let newRootReducer | ||
const replaceReducerSpy = (reducer, ...args) => { | ||
newRootReducer = reducer | ||
return store.replaceReducer(reducer, ...args) | ||
} | ||
const storeProxy = { ...store, replaceReducer: replaceReducerSpy } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also include a 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// Call modifyStore | ||
this.cleanup = modifyStore(props.modification, storeProxy, storeState) | ||
|
||
// Check if replaceReducer was called, patch new state | ||
if (newRootReducer) { | ||
const patchedState = newRootReducer(storeState, {}) | ||
this.patchedContextValue = { | ||
...context, | ||
storeState: patchedState | ||
} | ||
} | ||
} | ||
componentWillUnmount() { | ||
if (this.cleanup) { | ||
const { modification } = this.props | ||
const { store, storeState } = this.context | ||
this.cleanup(modification, store, storeState) | ||
} | ||
} | ||
render() { | ||
// We only want to Provide a new context-value if it has actually been patched | ||
// AND this render has the same context as when component was constructed | ||
// This is a heuristic for the first render-pass. Even when heuristic is wrong, it | ||
// is still safe to patch context as long as the original context has not changed. | ||
if (this.patchedContextValue && this.firstContext === this.context) { | ||
return ( | ||
<Context.Provider value={this.patchedContextValue}> | ||
{this.props.children} | ||
</Context.Provider> | ||
) | ||
} | ||
return this.props.children | ||
} | ||
} | ||
|
||
StoreModifier.contextType = Context | ||
StoreModifier.propTypes = { | ||
modification: PropTypes.object.isRequired, | ||
modifyStore: PropTypes.func, | ||
children: PropTypes.any | ||
} | ||
|
||
return StoreModifier | ||
} | ||
|
||
export default createStoreModifier() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
import React from 'react' | ||
import hoistStatics from 'hoist-non-react-statics' | ||
import StoreModifier, { createStoreModifier } from './StoreModifier' | ||
|
||
export default function withModifiedStore(modification, options = {}) { | ||
const ModifierComponent = options.context | ||
? createStoreModifier(options.context) | ||
: StoreModifier | ||
|
||
return function wrapWithStoreModifier(WrappedComponent) { | ||
const wrappedComponentName = | ||
WrappedComponent.displayName || WrappedComponent.name || 'Component' | ||
|
||
const displayName = `StoreModifier(${wrappedComponentName})` | ||
|
||
function StoreModifierHOC({ forwardedRef, ...props }) { | ||
return ( | ||
<ModifierComponent | ||
modification={modification} | ||
modifyStore={options.modifyStore} | ||
> | ||
<WrappedComponent {...props} ref={forwardedRef} /> | ||
</ModifierComponent> | ||
) | ||
} | ||
|
||
StoreModifierHOC.WrappedComponent = WrappedComponent | ||
StoreModifierHOC.displayName = displayName | ||
|
||
if (options.forwardRef) { | ||
const forwarded = React.forwardRef(function forwardStoreModifierRef( | ||
props, | ||
ref | ||
) { | ||
return <StoreModifierHOC {...props} forwardedRef={ref} /> | ||
}) | ||
|
||
forwarded.WrappedComponent = WrappedComponent | ||
forwarded.displayName = displayName | ||
|
||
return hoistStatics(forwarded, WrappedComponent) | ||
} | ||
|
||
return hoistStatics(StoreModifierHOC, WrappedComponent) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a huge fan of having users write this function like this, perhaps we can go with something like the chaining method?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 calledapplyModifiers
? 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
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There is a very strong similarity here between
actions
/dispatch
/subscribe
andmodifier
/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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.