Refactor react components#120
Conversation
|
|
||
| return renderComponent({ | ||
| name: 'Consumer', | ||
| renderProps: props, |
There was a problem hiding this comment.
Сюда можно целиком прокинуть this.props. Вообще это общая рекомендация по PR - по максимуму избавиться от всяких новомодных штук типа спредов и рестов.
| appState: null | ||
| } | ||
|
|
||
| componentWillMount() { |
There was a problem hiding this comment.
Нужен getDerivedStateFromProps, который будет обновлять стейт на изменение map.
| import { Subscription } from '../subscription/subscription' | ||
|
|
||
| export type Context = { state?: any; app?: Stapp<any, any> } | ||
| export type Context = { app: Stapp<any, any> } |
| } | ||
| }) => | ||
| createElement(StappContext.Provider, { | ||
| value: { app }, |
There was a problem hiding this comment.
вообще, если остается только app, наверное стоит напрямую его в value передавать
| export type Context = { app: Stapp<any, any> } | ||
|
|
||
| export const StappContext = createContext<Context>({}) | ||
| export const StappContext = createContext<Context>({} as any) |
There was a problem hiding this comment.
и тогда будет createContext<Stapp<any, any>| null>(null)
| componentWillMount() { | ||
| const app = this.props.app | ||
|
|
||
| this.subscription = app.subscribe(this.setAppState.bind(this)) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| import shallowEqual from 'fbjs/lib/shallowEqual' | ||
| import { Component, ComponentClass, createElement } from 'react' | ||
| import { identity } from 'stapp/lib/helpers/identity/identity' | ||
| import { ConsumerClass } from '../context/ConsumerClass' |
There was a problem hiding this comment.
И действительно стоит это переименовать, например AppSubscription
…or AppSubscription using getDerivedStateFromProps
4dc6258 to
b940b1e
Compare
| this.subscription = app.subscribe((state: State) => this.setAppState(state)) | ||
| } | ||
|
|
||
| static getDerivedStateFromProps( |
There was a problem hiding this comment.
https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops
Note that this method is fired on every render, regardless of the cause.
Может, подумаем всё-таки над другим способом. Два мапа и shallowEqual на каждый рендер - перебор.
There was a problem hiding this comment.
Посмотри, как сделано в create-subscription
https://github.com/facebook/react/blob/master/packages/create-subscription/src/createSubscription.js#L70
На самом деле я бы почти всё сделал как в классе, который возвращает createSubscription, только еще map добавить
| const appState = this.state.appState | ||
|
|
||
| return renderComponent({ | ||
| name: 'Consumer', |
There was a problem hiding this comment.
Название другое должно быть
|
|
||
| export type AppSubscriptionState<Result> = { appState?: Result } | ||
|
|
||
| export class AppSubscription<State, Api, Result> extends Component< |
There was a problem hiding this comment.
StappSubscription мне как-то больше нравится :/
…or 'map' changed in getDerivedStateFromProps
…tion state inside constructor, move 'map' default value to StappSubscription.defaultProps
No description provided.