Add mobx models #8
Conversation
src/models/ItemModel.ts
Outdated
| get isRemoving() { | ||
| return this._isRemoving | ||
| } | ||
|
|
||
| set isRemoving(isRemoving) { | ||
| this._isRemoving = isRemoving; | ||
| } |
There was a problem hiding this comment.
Я бы сделал так, чтобы не создавать 4 булевых переменных для каждого статуса и не пытаться жонглировать ими внутри экшнов:
type Status = 'idle' | 'pending' | 'success' | 'error'
class ItemModel {
private removingStatus: Status;
// ...
setRemovingStatus(status: Status) {
this.removingStatus = status;
}
get isRemoving() {
return this.removingStatus === 'pending'
}
}There was a problem hiding this comment.
А где ты видишь 4 булевых переменных?) Это не то что ты подумал кажется, это индикатор открыта ли модалка)
There was a problem hiding this comment.
Ок, если так, то достаточно:
// store:
public isRemoving: boolean;
setIsRemoving(isRemoving: boolean) {
this.isRemoving = isRemoving;
}
// consumer:
ItemModel.isRemovingThere was a problem hiding this comment.
Как ты видишь, я не фанат лишних геттеров. Если он лишь отдает состояние приватной переменной, то незачем добавлять сложность :D
| items: Item[]; | ||
| } | ||
|
|
||
| export class ItemListModel { |
There was a problem hiding this comment.
Модель – это лишь часть стора. Что-то, что перечислено в следующих трех строчках, что можно записать в DTO-like интерфейс и что можно сделать базой для стора вот так:
interface ItemListModel {
_items: ItemModel[];
_onlyChecked: boolean;
_removingId: number | null;
}
export class ItemListStore implements ItemListModel {
protected _items = [];
protected _onlyChecked = false;
protected _removingId = null;
...
}Mobx-state-tree (opinionated обертка над MobX) тоже считает так: https://mobx-state-tree.js.org/concepts/trees#creating-models
There was a problem hiding this comment.
Поэтому предлагаю переименовать стор, например, в ItemListStore
There was a problem hiding this comment.
Я не понимаю что ты понимаешь под моделью. И не понимаю концепцию стора в Mobx, нафига он там нужен) Гляну, почитаю.
There was a problem hiding this comment.
Стор – просто любая observable-сущность с данными, которая создается через makeAutoObservable или ее аналог
There was a problem hiding this comment.
Модель, вью (пропертис и их компьютеды), и экшны – части стора.
There was a problem hiding this comment.
Почитаю и если че обсудим голосом. Это какая-то терминология из какой-то конкретной метологии конкретно) Видимо словарик нужен что бы не путаться
| this._ids = [...this._ids, id]; | ||
|
|
||
| return () => { | ||
| this._ids = this._ids.filter((checkingId) => checkingId !== id); |
There was a problem hiding this comment.
А это разве будет работать без runInAction (src)?
There was a problem hiding this comment.
Кажется, да, но будет спамить ворнинги в консоль, ведь это другая функция, не экшн.
| private _ids: string[] = []; | ||
|
|
||
| constructor() { | ||
| makeAutoObservable(this); |
There was a problem hiding this comment.
Предлагаю сразу привязывать контекст для всех методов через makeAutoObservable(this, undefined, {autoBind: true}).
По моему опыту, у этого нет недостатков, и в следующем MobX 7 это, возможно, будет сделано по умолчанию: mobxjs/mobx#3796
There was a problem hiding this comment.
Это на усмотрение. В рамках демки это не важно)
Так звучит разумно, только что бы не помнить каждый раз и не писать в каждой модели обертку надо сделать просто типа makeAutoBindObservable
| @@ -100,15 +93,40 @@ export const ListPage: FC = () => { | |||
| <li>Come back here and check that data will update</li> | |||
| </ul> | |||
| <ConfirmModal | |||
| show={!!removingItem} | |||
| show={!!itemListModel.removingId} | |||
There was a problem hiding this comment.
Давай отдавать сюда компьютед, чтобы пользователь (ListPage) не видел внутренности стора, а пользовался понятным API. Что-то вроде:
// ItemListModel.ts
get canRemoveItem() {
return this.removingId !== null;
}
// ListPage.tsx
<ConfirmModal show={itemListModel.canRemoveItem} ... />There was a problem hiding this comment.
Это просто разный подходы) Можем отдельно подискутировать по этому поводу.
| .finally(hideLoader); | ||
| }} | ||
| >Save</Button> | ||
| <Button | ||
| className='m-1' | ||
| variant='danger' | ||
| onClick={() => setRemoveConfirmationOpened(true)} | ||
| onClick={() => itemModel.isRemoving = true} |
There was a problem hiding this comment.
Тут тоже надо менять состояние стора через экшн.
| export const ItemPage: FC = observer(() => { | ||
| const { id } = useParams<{ id: string }>(); | ||
| const loader = useLoaderModel(); | ||
|
|
||
| const {data, refetch, isFetching, isError: isItemError} = useQuery({ | ||
| queryKey: ['item', id], | ||
| queryFn: () => api.getItem(Number(id)), | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (isFetching) { | ||
| return loader.show(); | ||
| } | ||
| }, [isFetching, loader]); | ||
|
|
||
| const [itemModel, setItemModel] = useState<ItemModel | null>(null); | ||
| const [error, setError] = useState<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if(isItemError || isErrorItem(data)) { | ||
| setError((data as ItemResponseError)?.error || 'Item data loading error'); | ||
| } else if(data) { | ||
| setItemModel(new ItemModel({ item: data })) | ||
| } | ||
| },[data, isItemError]) | ||
|
|
||
| if (error) { | ||
| return ( | ||
| <div style={{ color: 'red' }}>{error}</div> | ||
| ) | ||
| } | ||
|
|
||
| if (!itemModel) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <ItemPageView itemModel={itemModel} refetchItem={refetch}/> | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Я понимаю, что код демонстративный, но тут точно надо хэндлить две асинхронных операции одновременно: из ItemModel и useQuery? Немного не понимаю, что здесь происходит =/
There was a problem hiding this comment.
Вот эта как раз часть приближена к реальному использованию. Обсудим голосом, это очень важно. Спасибо!
| return new ItemListModel({ items: items || [] }); | ||
| }, [items]); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Кстати, если передаем стор ниже, то можно на уровне страницы положить его в провайдер, чтобы не проп дриллить. Но это уже по ситуации. Если вложенность небольшая, то легче будет пару раз передать как проп.
There was a problem hiding this comment.
Все так, зависит от ситуации. Тут простое правило - заменять props drilling на провайдер когда глубина больше двух) А сразу смысла нет - потому что ты никогда не видел новогоднюю елку из провайдеров? Когда там аля 10 вложенных провайдеров? Я видел)
There was a problem hiding this comment.
Такое колличество провайдеров держать в голове нереально) Кароч провайдеры нужно юзать когда это действительно необходимо
| }; | ||
|
|
||
| export const LoaderProvider: FC<Props> = (props) => { | ||
| const loaderModel = useMemo(() => { |
There was a problem hiding this comment.
Тут тоже можно заменить на useState :)
There was a problem hiding this comment.
Вкусовщина) Просто разные подходы
src/models/ItemListModel.ts
Outdated
| } | ||
|
|
||
| add(item: Item) { | ||
| this._items = [...this._items, new ItemModel({ item })]; |
There was a problem hiding this comment.
Или просто this._items.push(new ItemModel({ item })), ведь одна из фишек мобикса – мутабельность.
There was a problem hiding this comment.
Вот помню раньше когда-то так плохо работало) Проверю
No description provided.