Skip to content

Redux#2

Merged
egorgrushin merged 5 commits intomasterfrom
redux
Jan 26, 2020
Merged

Redux#2
egorgrushin merged 5 commits intomasterfrom
redux

Conversation

@egorgrushin
Copy link
Copy Markdown
Owner

add redux,
move books to redux state instead of useState of Home

@egorgrushin egorgrushin requested a review from DeyLak January 25, 2020 16:57
"@typescript-eslint/interface-name-prefix": "off",
"@typescript-eslint/no-inferrable-types": "off",
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/no-empty-interface": "off",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Зачем ты отключил это не совсем понятно

import { AnyAction } from 'redux';

export enum Actions {
AddList = '[Entities] Add List',
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Странноватый нейминг, скорее все таки принят UPPER_SNAKE_CASE

@@ -0,0 +1,14 @@
import { AnyAction } from 'redux';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Тайпинг для пейлоада бы тоже прописать

const { entities, key } = action.payload;
return {
...state,
[key]: (state[key] || []).concat(entities),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Я бы написал [...(state[key] || []), ...entities]

isInterrupted?: boolean;
}

export const affectState = <T, E = any>(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Абсолютно не понимаю, почему в ts принято буквами называть типы в дженериках, я стараюсь давать им нормальные названия. Ну кроме может быть T, но если их больше одного, то всегда называю как-то нормально.

};

const refreshFn = (): void => {
paginator.next(false);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Странноватое апи немного с флагом, не особо читается, что происходит. Было бы удобней какое-то явное апи типо loadNextPage, clearPages. Может быть его возвращать из хука. У меня в либе оно в класс обернуто.

toIndex: number,
): Observable<IBook[]> => from(fetchBooks(value, fromIndex, toIndex));

const debouncedFilter$ = filter$.pipe(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Каждый раз создавать такой - не очень

<div {...{ className: styles.wrapper }}>
{items.map((item) => {
<div {...{ className: classNames(styles.wrapper, className) }}>
{items?.map((item) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Заглушку бы какую-нибудь добавить, если нет items

export const store = createStore(
rootReducer,
{},
(window as any).__REDUX_DEVTOOLS_EXTENSION__ && (window as any).__REDUX_DEVTOOLS_EXTENSION__(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

можно ?.() написать

return { type: Actions.AddList, payload: { entities, key } };
};

export const ReplaceList = <T>(entities: T[], key: string): AnyAction => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Не хватает всякой работы с объектами по id, нормализации и прочих плюшек

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

потому что все это в либе, не хочу еще раз этот путь проходить заного

@egorgrushin egorgrushin merged commit 80af6f3 into master Jan 26, 2020
egorgrushin added a commit that referenced this pull request Nov 20, 2024
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.

2 participants