Skip to content

Add connected generic component example - #358

Open
libracapitalinvestments-rgb wants to merge 1 commit into
piotrwitek:masterfrom
libracapitalinvestments-rgb:casharmy/fix-issue-55
Open

Add connected generic component example#358
libracapitalinvestments-rgb wants to merge 1 commit into
piotrwitek:masterfrom
libracapitalinvestments-rgb:casharmy/fix-issue-55

Conversation

@libracapitalinvestments-rgb

Copy link
Copy Markdown

Adds an example demonstrating how to combine generic components with React-Redux's connect(). This addresses the challenge of TypeScript's limitation with generic type parameters in connect() calls by using a wrapper function pattern. Closes #55

@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 a factory function pattern to connect generic React components with React-Redux's connect(), complete with documentation, implementation, and usage examples. The review feedback highlights two key areas for improvement: first, making the state type generic in the factory function to avoid unsafe type assertions in consumer components; second, introducing a keyExtractor prop to avoid using array indices as React keys, which is a known anti-pattern. The feedback also includes necessary updates to the usage examples and documentation to align with these improvements.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +34 to +42
export function connectGenericList<T>(
mapStateToProps: (state: RootState, ownProps: OwnProps<T>) => StateProps<T>
) {
// We use a cast here because connect cannot directly handle generic components.
// The result is a connected component typed for the specific T.
return connect<StateProps<T>, {}, OwnProps<T>, RootState>(mapStateToProps)(
GenericList as new (props: GenericListProps<T>) => GenericList<T>
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The factory function connectGenericList is currently coupled to the global RootState type. This limits its reusability and forces consumers to use unsafe type assertions (like state as any as ExampleState in the usage example) if their local state or store slice differs. Making the state type generic (e.g., S = RootState) makes the factory function truly reusable and type-safe.

Suggested change
export function connectGenericList<T>(
mapStateToProps: (state: RootState, ownProps: OwnProps<T>) => StateProps<T>
) {
// We use a cast here because connect cannot directly handle generic components.
// The result is a connected component typed for the specific T.
return connect<StateProps<T>, {}, OwnProps<T>, RootState>(mapStateToProps)(
GenericList as new (props: GenericListProps<T>) => GenericList<T>
);
}
export function connectGenericList<T, S = RootState>(
mapStateToProps: (state: S, ownProps: OwnProps<T>) => StateProps<T>
) {
// We use a cast here because connect cannot directly handle generic components.
// The result is a connected component typed for the specific T.
return connect<StateProps<T>, {}, OwnProps<T>, S>(mapStateToProps)(
GenericList as new (props: GenericListProps<T>) => GenericList<T>
);
}

Comment on lines +30 to +35
const mapStateToProps = (state: RootState) => ({
items: (state as any as ExampleState).todos,
});

// Create a connected generic list typed for Todo
export const ConnectedTodoList = connectGenericList<Todo>(mapStateToProps);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

With the generic state parameter added to connectGenericList, we can type the state parameter in mapStateToProps directly as ExampleState and avoid the unsafe double type assertion (state as any as ExampleState).

Suggested change
const mapStateToProps = (state: RootState) => ({
items: (state as any as ExampleState).todos,
});
// Create a connected generic list typed for Todo
export const ConnectedTodoList = connectGenericList<Todo>(mapStateToProps);
// Map state to props for Todo items
const mapStateToProps = (state: ExampleState) => ({
items: state.todos,
});
// Create a connected generic list typed for Todo
export const ConnectedTodoList = connectGenericList<Todo, ExampleState>(mapStateToProps);

Comment on lines +6 to +9
export interface GenericListProps<T> {
items: T[];
itemRenderer: (item: T) => JSX.Element;
}

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

Using the array index as a React key is an anti-pattern that can lead to rendering bugs and performance issues when the list is reordered or mutated. Since this is a generic component, we should allow the consumer to provide a custom key extractor function to uniquely identify each item.

Suggested change
export interface GenericListProps<T> {
items: T[];
itemRenderer: (item: T) => JSX.Element;
}
export interface GenericListProps<T> {
items: T[];
itemRenderer: (item: T) => JSX.Element;
keyExtractor?: (item: T, index: number) => string | number;
}

Comment on lines +12 to +20
function GenericList<T>(props: GenericListProps<T>) {
return (
<ul>
{props.items.map((item, i) => (
<li key={i}>{props.itemRenderer(item)}</li>
))}
</ul>
);
}

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

Update the GenericList component to use the optional keyExtractor prop, falling back to the array index only if a custom extractor is not provided.

Suggested change
function GenericList<T>(props: GenericListProps<T>) {
return (
<ul>
{props.items.map((item, i) => (
<li key={i}>{props.itemRenderer(item)}</li>
))}
</ul>
);
}
function GenericList<T>(props: GenericListProps<T>) {
const { items, itemRenderer, keyExtractor } = props;
return (
<ul>
{items.map((item, i) => {
const key = keyExtractor ? keyExtractor(item, i) : i;
return <li key={key}>{itemRenderer(item)}</li>;
})}
</ul>
);
}

Comment on lines +23 to +25
export interface OwnProps<T> {
itemRenderer: (item: T) => JSX.Element;
}

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

Add the optional keyExtractor prop to OwnProps<T> so that it can be passed through the connected component wrapper.

Suggested change
export interface OwnProps<T> {
itemRenderer: (item: T) => JSX.Element;
}
export interface OwnProps<T> {
itemRenderer: (item: T) => JSX.Element;
keyExtractor?: (item: T, index: number) => string | number;
}

Comment on lines +40 to +44
<ConnectedTodoList
itemRenderer={(todo: Todo) => (
<span key={todo.id}>{todo.text}</span>
)}
/>

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

Use the new keyExtractor prop to uniquely identify each item in the list, and remove the redundant key prop from the inner <span> inside itemRenderer.

    <ConnectedTodoList
      keyExtractor={(todo: Todo) => todo.id}
      itemRenderer={(todo: Todo) => (
        <span>{todo.text}</span>
      )}
    />

Comment on lines +46 to +52
export function connectGenericList<T>(
mapStateToProps: (state: RootState, ownProps: OwnProps<T>) => StateProps<T>
) {
return connect<StateProps<T>, {}, OwnProps<T>, RootState>(mapStateToProps)(
GenericList as new (props: GenericListProps<T>) => GenericList<T>
);
}

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

Update the factory function definition in the documentation to match the generic state parameter improvement.

Suggested change
export function connectGenericList<T>(
mapStateToProps: (state: RootState, ownProps: OwnProps<T>) => StateProps<T>
) {
return connect<StateProps<T>, {}, OwnProps<T>, RootState>(mapStateToProps)(
GenericList as new (props: GenericListProps<T>) => GenericList<T>
);
}
export function connectGenericList<T, S = RootState>(
mapStateToProps: (state: S, ownProps: OwnProps<T>) => StateProps<T>
) {
return connect<StateProps<T>, {}, OwnProps<T>, S>(mapStateToProps)(
GenericList as new (props: GenericListProps<T>) => GenericList<T>
);
}

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.

suggestion: connected generic component

1 participant