meteor / todos

The example app "Todos", written following the Meteor Guide
Other
535 stars 367 forks source link

React: Using State instead of Session #128

Closed joncursi closed 6 years ago

joncursi commented 8 years ago

In the react example, it looks like Session is being used (abused?) to mimic / shortcut the passing of React state through the component tree. An example of this is the menuOpen flag, which is set deep within the MobileMenu component: https://github.com/meteor/todos/blob/react/imports/ui/components/MobileMenu.jsx#L6

MobileMenu is rendered as the child of a long component tree:

AppContainer -> App (Layout) -> ListContainer -> ListPage -> ListHeader -> MobileMenu

By using session, you are able to bypass the need to pass menuOpen state up this component tree to AppContainer, and can set changes directly within the AppContainer: https://github.com/meteor/todos/blob/react/imports/ui/containers/AppContainer.jsx#L15

This is really convenient, but also seems to be a huge anti-pattern in React. Can you provide some insight on how this should be (properly?) done in React (i.e. if Session were not available)?

Using React Router's {this.props.children} to render page components within a parent layout, I am unsure of how to pass state from the parent layout -> child page (how do you set those props?), and even more-so, from child page -> parent layout. Related issue: https://github.com/reactjs/react-router/issues/1857

tmeasday commented 8 years ago

Hey @joncursi.

It's a good question. You can see by the "XXX: no session" comment in that file that we don't actually think this is the best approach.

I think the answer to this is either:

a) Pass a callback down the render tree to change the state on the relevant parent element (AppContainer in this case). So MobileMenu would receive a prop called onMenuOpenChanged or the like.

b) Use a "global" state management solution like Redux.

I would love some good documentation about idiomatic patterns for using Redux, React and Meteor together (with an accompanying guide article: https://github.com/meteor/guide/issues/234). It's not something that I can focus on right now but it's a great area for a community member who is already doing it in their app to step up. Even a PR against the todos/react branch would be very valuable I expect.

hwillson commented 6 years ago

The latest React version of todos takes a more global state management based approach, by leveraging a ReactiveVar instead of Session. The ReactiveVar is passed down as a prop when needed, which is somewhat similar to how a Redux store is passed around as a prop. There are differences (as the Redux store is usually only passed as far as a container component, at which point the needed state is extracted and passed further down into presentational components), but the current ReactiveVar approach gets rid of the global Session object, while maintaining todos simplicity.

We will likely want to take this further in the future, but Redux itself might not be the best approach (since there are now other options that are picking up steam, especially around React's new Context API). Closing for now - thanks!