cryptag / leapchat

Ephemeral, encrypted, in-browser chat rooms
https://www.leapchat.org/
Other
184 stars 32 forks source link

Move component and redux state down the component tree where possible #303

Closed jimmcgaw closed 1 year ago

jimmcgaw commented 1 year ago

What's in this PR:

  1. Converts several full class components into functional component equivalents.
  2. Move reads of the redux store down the component tree, instead of passing them down from the root.
  3. Move state down the tree as much as possible.

The key things are 2 and 3. What the React upgrade gave us, first and foremost, is the ability to quickly and easily encapsulate state in small functional components. Much of the prop drilling is eliminated if you just move the state into leaf nodes in the component tree, or at least as far from the root as is allowed.

I also didn't realize you could wrap child components in the redux store, and give them access to the subset of the state or dispatch methods that they need. Previously, we were throwing all of them into the root component and drilling them down.

This is a good first step. Next step is to actually determine the layout of this page, in its various forms, on both mobile and desktop. (For example, I imagine we'll introduce a right panel for the users list, instead of having it in the header.) Then, once that's sketched out, design the component tree so that state locality follows the cohesion design principle as much as possible. Where that is tricky to do, then we can explore using contexts.

elimisteve commented 1 year ago

Much of the prop drilling is eliminated if you just move the state into leaf nodes in the component tree, or at least as far from the root as is allowed.

In my experience, components often end up needing to know something about each other's state, and it's hard for them to get that information if a lot of state is local.

But if there is one global state store that components directly reference and pull from, then every component has the information it needs. Plus this eliminates prop drilling because no component gets state from its parent.

Example: if a task is clicked on, what should happen? We should show that task's details... unless the Assignee dropdown is open. But if only the Assignee component knows whether it's open, its parent won't know, and thus won't know how to behave when clicked.

Example 2: when most state is local, then when we take the user to, say, a different tab in the Right Panel, then take them back, the UI will change! Collapsed todo lists will reset to being uncollapsed, a filtered todo list will reset to being unfiltered, etc. (Though we could solve this by putting "sub-tabs" (e.g., Task Details) on top of the tab's (e.g., Todo Lists) UI so that what's underneath stays in the DOM! I did this for Effective so that the amount that the user scrolled down the right panel would be preserved, which of course would preserve all the local React state. Hmm...)

With Easy Peasy we can pluck out just the global state we want to use, then use it -- no prop drilling required: https://easy-peasy.vercel.app/docs/tutorials/quick-start.html#using-state-in-your-components

The global store can also include setters so that they never need to be passed up or down the component hierarchy. This code example may be a microcosm of exactly what we want to do, since it uses some local state via useState and also some global state via Easy Peasy! https://easy-peasy.vercel.app/docs/tutorials/quick-start.html#dispatching-actions

I guess it could be too inefficient to make almost everything global, due to excessive re-rendering...

Shall we default to local state in most cases, but try to foresee upcoming dependencies so as to avoid the problem of "the left hand doesn't know what the right hand's doing" (but needs to)?

jimmcgaw commented 1 year ago

Much of the prop drilling is eliminated if you just move the state into leaf nodes in the component tree, or at least as far from the root as is allowed.

In my experience, components often end up needing to know something about each other's state, and it's hard for them to get that information if a lot of state is local.

But if there is one global state store that components directly reference and pull from, then every component has the information it needs. Plus this eliminates prop drilling because no component gets state from its parent.

Example: if a task is clicked on, what should happen? We should show that task's details... unless the Assignee dropdown is open. But if only the Assignee component knows whether it's open, its parent won't know, and thus won't know how to behave when clicked.

Example 2: when most state is local, then when we take the user to, say, a different tab in the Right Panel, then take them back, the UI will change! Collapsed todo lists will reset to being uncollapsed, a filtered todo list will reset to being unfiltered, etc. (Though we could solve this by putting "sub-tabs" (e.g., Task Details) on top of the tab's (e.g., Todo Lists) UI so that what's underneath stays in the DOM! I did this for Effective so that the amount that the user scrolled down the right panel would be preserved, which of course would preserve all the local React state. Hmm...)

With Easy Peasy we can pluck out just the global state we want to use, then use it -- no prop drilling required: https://easy-peasy.vercel.app/docs/tutorials/quick-start.html#using-state-in-your-components

The global store can also include setters so that they never need to be passed up or down the component hierarchy. This code example may be a microcosm of exactly what we want to do, since it uses some local state via useState and also some global state via Easy Peasy! https://easy-peasy.vercel.app/docs/tutorials/quick-start.html#dispatching-actions

I guess it could be too inefficient to make almost everything global, due to excessive re-rendering...

Shall we default to local state in most cases, but try to foresee upcoming dependencies so as to avoid the problem of "the left hand doesn't know what the right hand's doing" (but needs to)?

I might not have been precise in my description. The thing I'm mainly alluding to is the state of modals. That's basically a lot of props being sent down to control show / hide from various places in the tree. I don't see any reason that needs to be thrown into the global state, but we could do that.

Regarding redux: there's still a global store. I've just made it explicit which components are taking which elements from the global state, instead of attaching everything to the root element and passing stuff down. We don't need to leave it this way, but it's a start, since it makes it clearer which elements need which data, which elements are mutating which data, and hence should be easier to convert to easy peasy, if that's what we end up doing.

elimisteve commented 1 year ago

Regarding redux: there's still a global store. I've just made it explicit which components are taking which elements from the global state, instead of attaching everything to the root element and passing stuff down. We don't need to leave it this way, but it's a start, ...

That's a huge improvement indeed!

jimmcgaw commented 1 year ago

@elimisteve As a rule of thumb, for PRs this massive (or any PR, but especially big ones), I'm game to jump on a screenshare call to review. I didn't want to annotate everything.