-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix: suggestion: connected generic component #232
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
base: master
Are you sure you want to change the base?
Changes from all commits
b84252f
6ce17b1
083aeff
27fb45a
e4c9ed3
59ba1db
07c3160
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,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> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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 = { | ||
|
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. |
||
| 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); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||||||||||||||||||
| import * as React from 'react'; | ||||||||||||||||||||||
| import { createStore } from 'redux'; | ||||||||||||||||||||||
|
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. |
||||||||||||||||||||||
| 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
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. The
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 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
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. Following the suggestion to improve
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** Usage */ | ||||||||||||||||||||||
| export const App: React.FC = () => ( | ||||||||||||||||||||||
| <Provider store={store}> | ||||||||||||||||||||||
| {/* `items` is injected by connect; only `itemRenderer` needs to be supplied */} | ||||||||||||||||||||||
| <ConnectedTodoList itemRenderer={todoItemRenderer} /> | ||||||||||||||||||||||
| </Provider> | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
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.
Using an item's index as a
keyis 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
GenericListcomponent more robust and adhere to best practices, let's delegate the responsibility of rendering the list item, including itskey, to theitemRenderer. This makes theGenericLista 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.