Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Redux selectors for all state accesses #1966

Closed
MaximusHaximus opened this issue Jul 26, 2021 · 4 comments · Fixed by #2178
Closed

Add Redux selectors for all state accesses #1966

MaximusHaximus opened this issue Jul 26, 2021 · 4 comments · Fixed by #2178
Assignees

Comments

@MaximusHaximus
Copy link
Contributor

Blocks #1239

PROBLEM:
Currently, we access redux state using direct references to the entire state tree/structure. For example, rather than having a selector getActiveAccountId(), we have a direct reference to state.account.accountId in Profile.js.

Using direct references has several drawbacks:

  • Our components know the exact structure of our state tree, so if we want to move some data around, we have to update all components that use that state tree
  • It's hard for engineers to discover what data is 'the same' across the codebase. Some code might destructure from the parent state, other places might reference it directly as a child of the parent state, and others might do full references from state.xxx.xxx.xxx all the way to the child data. As a result, trying to find all places that reference a specific piece of data is challenging and error prone. By always using selectors to select logically identical data from the store, we can easily audit every component that is linked to that piece of data with a single, unique string (the name of the selector)
  • We cannot memoize access to the data that is computed-on-selection from Redux. For example, if a component needs data from Redux to be sorted differently than it is in Redux, it is common to select the data from Redux, then create a new copy of that data sorted in the new order that we want it to be displayed in. This pattern leads to unnecessary re-renders in cases where the data that we are loading from Redux has not actually changed, but we still create new copies of the data each time we re-render, so all child components re-render because the identity of the sorted data has changed. With memoized selectors, the results of the selectors do not change unless the source data that the selector is based off of has changed.

SOLUTION:
We should always use selectors to load data from our Redux store, with no exceptions.

@marcinbodnar
Copy link
Contributor

@MaximusHaximus I will start with it soon. Just a thought. What do you think about moving the selectors into separate files? I mean when I looked into this file:
https://github.com/near/near-wallet/blob/9563798e30f6305a17c0995042e8081baa67e692/src/reducers/nft/index.js
for the first time I thought that it would be great to have selectors separate, so we will have a good view of what is selector what is action. Most of our reducers (all of them in near future) are in separate folders, we can have structures like: reducers/nft/index.js and reducers/nft/selectors.js .

@MaximusHaximus
Copy link
Contributor Author

MaximusHaximus commented Jul 30, 2021

The selectors directly read data from the reducer state and shouldn't really have any other external dependencies, so I think that co-locating them in the same file is convenient and relevant.

Now that we are creating "slices" using redux-toolkit's createSlice(), action creators for simple reducer cases will be automatically created and exported as part of the slice along with the reducer itself. createSlice() automatically creates the action creators for every key defined in its reducers property hash. So we won't have a dedicated reducers directory anymore, but rather a slices directory that contains 1 file per slice, where a slice has not only the reducer but also simple actions attached to it.

Essentially, by using createSlice(), we are implicitly guided into domain-driven file structure rather than using different files for each 'piece' of the Redux ecosystem (action/reducer/selector) that makes up a domain.

This means that in almost every case (all cases where we don't have multiple reducers that react to the same action, which should be very rare), we can define everything (actions, reducers, selectors and thunks) relating to a particular domain in the same slice file, and export them all from one place.

Right now the slice file exports:

default export - the slice object itself; it has a reducer property and an actions property. Any actions that were defined in the slice using the reducers: {...} property will be exported as actions on the slice. Note that async thunks will not be properties on the slice; they are independent actions.

Named exports

  1. actions -- an object with slice.actions and all thunk actions that are specific to that slice spread into the same hash. See https://github.com/near/near-wallet/blob/9563798e30f6305a17c0995042e8081baa67e692/src/reducers/nft/index.js#L82-L81
  2. reducer -- the reducer property from the slice
  3. Each selector (selectXXXXyyyyyXXXX)

Selectors names are prefixed with select by convention, which should make it immediately clear which exports from the file are selectors, but we could export them all under a named export called selectors to make it even more obvious if you think that would be valuable 👍

That being said, while I think it makes a lot of sense to keep the selectors co-located with the state (reducers) that they primarily select from, there are two things that our current first implemented slice exports directly from the slice file that we may want to pull out into separate files due to either cross-domain coupling issues or technical challenges from a code structure standpoint:

A. Cross-domain thunks

  • Thunks may dispatch multiple actions that impact different reducers/slices of state, so it may make sense to actually create a directory just for crossDomainThunks, and group our thunks there by the app behaviour they provide rather than try to co-locate them with their slices, which won't make sense for thunks that dispatch actions that impact multiple slices.
    B. Cross-cutting actions
  • If we have an action that multiple slices/reducers match and react to, then we could end up with a circular reference by having actions exported from the slice file; all it would take is for a pair of slices to need to react to at least one action that is exported by the other slice, and we have a circular reference. We could define any/all cross-cutting actions in a directory named as such (crossCuttingActions?) to avoid the circular reference issue and to make it clear that those actions, when dispatched, mutate multiple slices of state, just from the fact that they are not being created and exported as part of their slice definition.

Since I don't have a good handle on if or how often these 2 cases will happen in practice for our codebase, and cross-cutting actions are strongly discouraged in general, I defaulted towards co-locating the things in the same file with the intention to only extract them into separate files/directories if we need to due to either (A) or (B) above :)

cc: @Patrick1904 @esaminu Feedback/suggestions welcome; this is where we get to make all the decisions about how our new and shiny redux-slice-based state will be structured at the codebase level :)

@esaminu
Copy link
Contributor

esaminu commented Jul 30, 2021

I like the domain driven file structure this will encourage 👍

While cross domain thunks and and cross cutting thunks might not be as common, I think cross domain selectors will be very common since I expect we'll often want to compute a value from different parts of the state tree to display things in a certain way or decide what to render. How we group cross cutting actions, selectors and thunks will end up being the bottom line here.

Other notes / opinions on this approach:

  • I think we should only use createAsyncThunk for things that need to handle a pending, fulfilled and rejected state. This is in the interest of keeping the state machine as simple as possible and not creating / dispatching actions that do not affect anything.
  • We should still encourage usage of local state and context where possible and not overuse redux state. Also in the interest of keeping state tree as simple as possible.
  • We need to consider the impact that using Immer will have on composability and testability of our actions, selectors and reducers. I don't think this will be much of an issue with actions but if we're using it for selectors then we need to be aware of the implications (if any) since selectors often need to be composed.

@MaximusHaximus
Copy link
Contributor Author

MaximusHaximus commented Aug 2, 2021

I like the domain driven file structure this will encourage 👍

While cross domain thunks and and cross cutting thunks might not be as common, I think cross domain selectors will be very common since I expect we'll often want to compute a value from different parts of the state tree to display things in a certain way or decide what to render. How we group cross cutting actions, selectors and thunks will end up being the bottom line here.

Agreed! We've already got cross-cutting selectors (things that need accountId from our accounts slice plus some data from another selector are gonna be common) and I expect it'll be the norm in many cases <3

Other notes / opinions on this approach:

  • I think we should only use createAsyncThunk for things that need to handle a pending, fulfilled and rejected state. This is in the interest of keeping the state machine as simple as possible and not creating / dispatching actions that do not affect anything.

There are essentially two types of async thunks:

  1. A thunk that directly mutates state with the result of an async call (e.g. the .fulfilled or .rejected payload is what we care about and use to mutate state)
  2. A thunk that dispatches other thunks to mutate state (we might not actually care about the overall result of this thunk, because each thunk that we dispatch may have dispatch their own actions and have their own error handling.

I think that we'll still want to use createAsyncThunk for cases like #2 where we need to dispatch multiple other actions that mutate state, even if we don't have explicit handlers for the .fulfilled, .pending and .rejected actions that createAsyncThunk() provide to us :)

  • We should still encourage usage of local state and context where possible and not overuse redux state. Also in the interest of keeping state tree as simple as possible.
  • Agreed; anything that doesn't need to be in Redux shouldn't be in Redux. Basically we need things that should persist across screen navigations. We're also intending to add redux-persist Soon ™️, so anything that we historically would've dropped directly into localStorage will also need to be in Redux once we've got redux-persist added :)
  • We need to consider the impact that using Immer will have on composability and testability of our actions, selectors and reducers. I don't think this will be much of an issue with actions but if we're using it for selectors then we need to be aware of the implications (if any) since selectors often need to be composed.
  • Thankfully, immer in reducers has no impact on other code because it returns plain javascript objects in every case :). It's not like immutable.js, where you constantly need to worry about what is in your reducer state because it is composed of a bunch of immutable.js objects :). I really like immer.js for this reason :)

marcinbodnar added a commit that referenced this issue Oct 1, 2021
marcinbodnar added a commit that referenced this issue Oct 1, 2021
marcinbodnar added a commit that referenced this issue Oct 1, 2021
marcinbodnar added a commit that referenced this issue Oct 1, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
andy-haynes pushed a commit that referenced this issue Oct 28, 2021
This was referenced Oct 29, 2021
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 a pull request may close this issue.

3 participants