Add typed connect factory example#307
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces documentation and code examples for typing connect factory functions in react-redux, which are used for per-instance memoization. The review feedback identifies that the makeMapStateToProps implementation in both the source code and the README fails to properly demonstrate the factory pattern because it updates a closure variable on every execution of the mapper function. The reviewer suggested refactoring the example to capture values from the initial props within the factory scope to correctly illustrate per-instance initialization.
| const makeMapStateToProps: MapStateToPropsFactory< | ||
| StateProps, | ||
| OwnProps, | ||
| Types.RootState | ||
| > = () => { | ||
| let offset = 0; | ||
|
|
||
| return (state, ownProps) => { | ||
| offset = ownProps.initialCount || 0; | ||
|
|
||
| return { | ||
| count: countersSelectors.getReduxCounter(state.counters) + offset, | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The factory function pattern in connect is most useful when you need to initialize something once per component instance, such as a memoized selector or capturing initial props. The current example updates the offset variable on every execution of the returned mapper, which defeats the purpose of using a factory closure. Capturing the initial prop value is a clearer demonstration of this pattern.
const makeMapStateToProps: MapStateToPropsFactory<
StateProps,
OwnProps,
Types.RootState
> = (_, initialOwnProps) => {
const offset = initialOwnProps.initialCount || 0;
return (state) => ({
count: countersSelectors.getReduxCounter(state.counters) + offset,
});
};
| const makeMapStateToProps: MapStateToPropsFactory< | ||
| StateProps, | ||
| OwnProps, | ||
| Types.RootState | ||
| > = () => { | ||
| let offset = 0; | ||
|
|
||
| return (state, ownProps) => { | ||
| offset = ownProps.initialCount || 0; | ||
|
|
||
| return { | ||
| count: countersSelectors.getReduxCounter(state.counters) + offset, | ||
| }; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The current implementation of makeMapStateToProps doesn't fully demonstrate the purpose of a factory function in connect. Factories are typically used to capture initial props or state for per-instance memoization. In this example, offset is being updated on every call of the returned mapper function, which makes the closure variable redundant. It's better to capture the initial value from initialOwnProps and return a mapper that doesn't depend on subsequent prop changes for that specific value.
| const makeMapStateToProps: MapStateToPropsFactory< | |
| StateProps, | |
| OwnProps, | |
| Types.RootState | |
| > = () => { | |
| let offset = 0; | |
| return (state, ownProps) => { | |
| offset = ownProps.initialCount || 0; | |
| return { | |
| count: countersSelectors.getReduxCounter(state.counters) + offset, | |
| }; | |
| }; | |
| }; | |
| const makeMapStateToProps: MapStateToPropsFactory< | |
| StateProps, | |
| OwnProps, | |
| Types.RootState | |
| > = (_, initialOwnProps) => { | |
| const offset = initialOwnProps.initialCount || 0; | |
| return (state) => ({ | |
| count: countersSelectors.getReduxCounter(state.counters) + offset, | |
| }); | |
| }; |
|
Addressed the factory-pattern review feedback in df79f83. The example now captures |
Closes #91
Summary
MapStateToPropsFactoryexample forconnectValidation
npm run ci-checkcd playground && npm run tsc