Skip to content

Add typed connect factory example#307

Open
narutamaaurum wants to merge 2 commits into
piotrwitek:masterfrom
narutamaaurum:fix-91-connect-factory-types
Open

Add typed connect factory example#307
narutamaaurum wants to merge 2 commits into
piotrwitek:masterfrom
narutamaaurum:fix-91-connect-factory-types

Conversation

@narutamaaurum

Copy link
Copy Markdown

Closes #91

Summary

  • add a typed MapStateToPropsFactory example for connect
  • include a small usage example in the playground
  • document the factory typing pattern in the README

Validation

  • npm run ci-check
  • cd playground && npm run tsc

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +15 to +29
const makeMapStateToProps: MapStateToPropsFactory<
StateProps,
OwnProps,
Types.RootState
> = () => {
let offset = 0;

return (state, ownProps) => {
offset = ownProps.initialCount || 0;

return {
count: countersSelectors.getReduxCounter(state.counters) + offset,
};
};
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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,
  });
};

Comment thread README.md
Comment on lines +1861 to +1875
const makeMapStateToProps: MapStateToPropsFactory<
StateProps,
OwnProps,
Types.RootState
> = () => {
let offset = 0;

return (state, ownProps) => {
offset = ownProps.initialCount || 0;

return {
count: countersSelectors.getReduxCounter(state.counters) + offset,
};
};
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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,
});
};

@narutamaaurum

Copy link
Copy Markdown
Author

Addressed the factory-pattern review feedback in df79f83. The example now captures initialCount from the factory's initial props and returns a mapper that reuses that per-instance value; README output was regenerated as part of the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Factory types for connect props

1 participant