Skip to content
58 changes: 58 additions & 0 deletions playground\src\components\generic-list-connected.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import * as React from 'react';
import { connect } from 'react-redux';

/** Generic List Component */
export type GenericListProps<T> = {
readonly items: T[];
readonly itemRenderer: (item: T) => JSX.Element;
};

export class GenericList<T> extends React.Component<GenericListProps<T>> {
render() {
const { items, itemRenderer } = this.props;
return (
<ul>
{items.map((item, idx) => (
<li key={idx}>{itemRenderer(item)}</li>
))}
</ul>
);
Comment on lines +13 to +19
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

Using an item's index as a key is an anti-pattern in React, especially for lists that can be reordered or have items added/removed. It can lead to performance issues and bugs with component state. A better practice is to use a unique and stable identifier from the data itself.

To make the GenericList component more robust and adhere to best practices, let's delegate the responsibility of rendering the list item, including its key, to the itemRenderer. This makes the GenericList a simpler wrapper and ensures the consumer, who knows the data structure, provides a correct key.

I'll suggest a corresponding change in generic-list-connected.usage.tsx.

    return (
      <ul>
        {items.map(itemRenderer)}
      </ul>
    );

}
}

/**
* Connected Generic List
*
* TypeScript does not allow parameterized generic type arguments in JSX, so
* `connect(...)(GenericList<Todo>)` is not valid syntax.
*
* Workaround:
* 1. Fix the generic type parameter via a concrete subclass
* 2. Connect the concrete subclass
*/

export type Todo = {
readonly id: number;
readonly title: string;
readonly completed: boolean;
};

type StoreState = {
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

To avoid duplicating type definitions, this StoreState type can be exported and then imported in generic-list-connected.usage.tsx. This improves maintainability.

export type StoreState = {

readonly todos: Todo[];
};

type OwnProps = {};

// Step 1 – Concrete subclass fixes the type parameter to `Todo`
class TodoList extends GenericList<Todo> {}

// Step 2 – mapStateToProps typed against the concrete props
const mapStateToProps = (
state: StoreState,
_ownProps: OwnProps
): Pick<GenericListProps<Todo>, 'items'> => ({
items: state.todos,
});

// Step 3 – connect the concrete subclass
export const ConnectedTodoList = connect(mapStateToProps)(TodoList);
39 changes: 39 additions & 0 deletions playground\src\components\generic-list-connected.usage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import * as React from 'react';
import { createStore } from 'redux';
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 createStore function from Redux is deprecated. The recommended approach is to use configureStore from @reduxjs/toolkit, which simplifies store setup and includes helpful development tools.

Suggested change
import { createStore } from 'redux';
import { configureStore } from '@reduxjs/toolkit';

import { Provider } from 'react-redux';

import { ConnectedTodoList, Todo } from './generic-list-connected';

/** Store Setup */
type StoreState = {
readonly todos: Todo[];
};
Comment on lines +5 to +10
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 StoreState type is duplicated from generic-list-connected.tsx. To improve maintainability, you should export it from generic-list-connected.tsx (I've left a separate comment for that) and import it here, removing the local definition.

Suggested change
import { ConnectedTodoList, Todo } from './generic-list-connected';
/** Store Setup */
type StoreState = {
readonly todos: Todo[];
};
import { ConnectedTodoList, Todo, StoreState } from './generic-list-connected';
/** Store Setup */


const initialState: StoreState = {
todos: [
{ id: 1, title: 'Learn TypeScript', completed: true },
{ id: 2, title: 'Read react-redux-typescript-guide', completed: false },
{ id: 3, title: 'Build something great', completed: false },
],
};

function reducer(state: StoreState = initialState): StoreState {
return state;
}

const store = createStore(reducer);

/** Item Renderer */
const todoItemRenderer = (item: Todo): JSX.Element => (
<span>
[{item.completed ? 'x' : ' '}] {item.title}
</span>
);
Comment on lines +27 to +31
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

Following the suggestion to improve GenericList, this todoItemRenderer should now be responsible for rendering the full list item (<li>) and providing a unique key. Since Todo items have a unique id, we can use that for the key.

Suggested change
const todoItemRenderer = (item: Todo): JSX.Element => (
<span>
[{item.completed ? 'x' : ' '}] {item.title}
</span>
);
const todoItemRenderer = (item: Todo): JSX.Element => (
<li key={item.id}>
[{item.completed ? 'x' : ' '}] {item.title}
</li>
);


/** Usage */
export const App: React.FC = () => (
<Provider store={store}>
{/* `items` is injected by connect; only `itemRenderer` needs to be supplied */}
<ConnectedTodoList itemRenderer={todoItemRenderer} />
</Provider>
);