Rethinking React-Redux folder structure

Hi everyone,

There have been a few changes in the recent years since we last discussed our folder structure conventions: Suggestion: Refactor the react_app folder structure
and maybe it’s time to adjust our conventions.

as mentioned in Code Structure | Redux,
there are a few common patterns that most Redux developers tend to use,
let’s start with the first one - Rails-style: separate folders for “actions”, “constants”, “reducers”, “containers”, and “components”**

I think it was inconvenient for developers to do all of the imports from the different directories,
and it basically led us after the discussion in Suggestion: Refactor the react_app folder structure to move into no. 2 on that list, “Feature folders” / “Domain”-style: separate folders per feature or domain, possibly with sub-folders per file type,
meaning most of our folders today look like this ( the example is with redux’s connect HOC style):

Component/
    // main file to export the redux-connected component and reducers
    index.js
    // the React component containing the JSX, life-cycle methods, and some logic.
    Component.js
    /** The Redux actions that are passed from the index.js into the connect HOC to the Component.js.
    * Since we started to use the API middleware, we see that often this file isn't needed
    * and some of the logic has moved into the Component.js file using React state.
    */
    ComponentActions.js
    /** same as the above, since using the API middleware, it's often not in use, the logic moved into 
    * Component.js inner state. It is needed in cases the component has more data than what the API 
    * returned to pass to other components. 
    */
    ComponentReducer.js
    /** selectors that are used in the index.js file to select attributes from Redux state and pass them to t 
    * the component can be re-used from other components.
    */
    ComponentSelectors.js
    // usually storing here the Actions types so it would be easier to re-use in reducers files.
    ComponentConstants.js
    // more logic that has moved from component.js to make the component.js file easier to read and test.
    ComponentHelpers.js
    /** in the tests directory we write tests for each exported function from the above files.
    * usually, it gets up to 5 different files
    */
    __tests__/

Redux-toolkit

Redux recommends the usage of “feature folders”, with all the Redux logic for a given feature in a single “slice/ducks” file".

they created a library that reduces the boilerplate of many files,
for example, the usage of createSlice: createSlice | Redux Toolkit
internally it uses their createAction and createReducer tying together Reducers, Actions and constants types in one file, meaning that we will combine 3 files in one.

Hooks

I think that the biggest change the React-Redux community had faced is the move to Hooks,
Hooks provide you the ability to write function components instead of class components by exposing the libraries’ inner context, they are way easier to write and I guess that’s why the community loves it so much.

examples of hooks:

// React
useState // for managing state
useEffect // lets you perform side effects in function components

// React-Redux
useSelector // Allows you to extract data from the Redux store state, using a selector function.
useDispatch // This hook returns a reference to the dispatch function from the Redux store. You may use it to dispatch actions as needed.
useStore // This hook returns a reference to the same Redux store that was passed into the <Provider> component.

this drastic change means that we move more logic into the Component.js file,
and no need to use the connect wrapper in the index.js file,
although the connect function might have better performance, Redux themselves recommend using hooks. see: Hooks | React Redux

I think our folders could look simpler, e.g:

Component/
    // export point for the all directory for easier imports. contain also the JSX, life-cycle methods, and some logic.
    index.js
    // Redux reducer logic and associated actions, I would write also the selectors and constants here if needed.
    componentSlice.js
    __tests__/

Writing tests

The final thing I would like to discuss is our tests conventions,
it makes it so hard to write 5/6 tests per component, even for a simple one,
The React community had moved almost entirely to use React Testing Library | Testing Library

The React Testing Library is a very light-weight solution for testing React components. It provides light utility functions on top of react-dom and react-dom/test-utils, in a way that encourages better testing practices. Its primary guiding principle is:

The more your tests resemble the way your software is used, the more confidence they can give you.

This approach doesn’t do tests-snapshots, which is the no. 1 factor to prevent a PR to get merged in time, because the PR becomes so big!

I believe we should move away from snapshotting into a more integration-tests approach.

Redux follows the same spirit: Writing Tests | Redux
and mention that Redux code can be treated as an implementation detail of the app, without requiring explicit tests for the Redux code in many circumstances.

this way we will also get rid of a few more unit test files and the tests will be more beneficial.

Waiting to hear your thoughts about it,
Thanks, Ron

6 Likes

Note that this was previously discussed:

Katello already uses React testing library and they are quite happy about it from what I heard.

1 Like

I would really love to simplify the boilerplate and this is goint in that direction :slight_smile:

Testing by snapshosts is terrible as I’ve said in the past, so big :+1: to move away whenever possible :slight_smile:

2 Likes

In general, big :+1: from me to moving to a slimmer structure with less boilerplate. Glad to see the redux community has finally figured out a way of reducing the amount of files and boilerplate needed to use it :slight_smile: I think the combination of hooks and slices actions can greatly simplify many components we have now that use the redux store (in addition to moving component-internal state out of the store, but that’s a separate discussion).
I also agree with reducing the number of tests that verify redux is working correctly and snapshots that often offer little value and make refactoring more difficult, although I think we already had this discussion and agreed to move to use react testing library.

A couple of notes/questions:

  • In case all (or most) of what the index.js file does is export the component, do we actually need an additional Component.js file?
  • We still have a few components that use the “old” rails convention, so that we didn’t even finish the migration from the previous convention to the current one. How do we approach this migration in a manner that allows us to complete it before a new convention arises and we have 3 different conventions instead of 2?
  • With regards to helpers, I would suggest we only extract code to a helpers.js once the main file becomes too big to manage or there is some common logic that is repeated in multiple sub-components, as jumping back and forth between files makes following the data flow more difficult.
  • What about components that have sub-components which are used only by them? will they live in a subfolder inside the main component? in a subfolder for each subcomponent? in a separate file inside the main component folder?
  • We might want to also reconsider the organization of the react_app folder itself, which could deserve a post of its own.

As a side note, I’ll mention the research that @Gwmngilfen did a few years ago, which indicated that a larger number of files in a PR correlated very well with the mean time-to-merge, even more than the number of lines changed. While correlation does not necessarily indicate causation, my personal experience as a reviewer certainly confirms that PRs with many files and lines changed take me a lot longer to review and tend to have a higher number of missed bugs. Anything that can reduce the amount of files needed to complete a feature/fix will likely also lead to easier reviews and faster merges.

3 Likes

Thanks @tbrisker! I will try to answer some of the points you mentioned

In case all (or most) of what the index.js file does is export the component, do we actually need an additional Component.js file?

I agree, I guess we could reduce one more file.

We still have a few components that use the “old” rails convention, so that we didn’t even finish the migration from the previous convention to the current one. How do we approach this migration in a manner that allows us to complete it before a new convention arises and we have 3 different conventions instead of 2?

Tough question, I guess it’s a matter of priority, and I wouldn’t ask devs to rewrite all of the components,
I guess that now as we are migrating more and more to PF4 we could refactor the whole folder,
but other than that, we should have some documentation on how to write a component and point it in new PRs, make everyone know this is the way we go… but I don’t see us refactoring old components honestly unless someone has the capacity for it of course.

With regards to helpers, I would suggest we only extract code to a helpers.js once the main file becomes too big to manage or there is some common logic that is repeated in multiple sub-components, as jumping back and forth between files makes following the data flow more difficult.

I am not sure what’s best, but personally, I would prefer to move helpers to helpers.js same as when we have subcomponent we would create a components/ dir for them usually.

Thanks @Ron_Lavi for bringing this up,
With the current structure, there’s a lot of boilerplate, reducing it and simplify the process would be great; Creating multiple files just for a simple component (excluding a few different tests) is a total pain.

However, changing fundamental conventions and folder structure would impact the entire application, and I’m not sure if we have the capacity for this kind of refactoring. We can use a new convention set only on new code, but this might make the codebase even more confusing. Developers tend to copy & paste existing parts, and having different structures might create a mess.

How about creating a set of foreman dedicated eslint rules? if someone try to copy and reuse outdated code (like the connect HOC) eslint creates a warning.

How about creating a set of foreman dedicated eslint rules? if someone try to copy and reuse outdated code (like the connect HOC) eslint creates a warning.

sounds like a good plan, even better then pointing newcomers to documentation :slight_smile:
We can set a specific rules only on changed files, something like:
"lint:script": "eslint -c eslintrc.js $(git diff --name-only --diff-filter=ACMRTUXB master | grep -E \"(.js$)\")" and on the error message link to our conventions docs

meanwhile I created a tracker for us with several tasks: Tracker #32903: Rethinking React-Redux folder structure - Foreman

another question, if there’s only one file in the tests directory, would it make sense just to move it upper into the component folder?
e.g:

component/
  index.js
  componentSlice.js
  component.test.js
1 Like

:+1: :+1: :+1: for less component files

I have an idea about naming the component files, now it’s like this:

RegistrationCommandsPage/index.js
RegistrationCommandsPage/RegistrationCommandsPage.scss
RegistrationCommandsPage/RegistrationCommandsPageActions.js
RegistrationCommandsPage/RegistrationCommandsPageHelpers.js
RegistrationCommandsPage/RegistrationCommandsPageSelectors.js

How about this?

RegistrationCommandsPage/index.js
RegistrationCommandsPage/styles.scss
RegistrationCommandsPage/actions.js
RegistrationCommandsPage/helpers.js
RegistrationCommandsPage/selectors.js

Do we really need the component name in each file name?

Also following the discussion from Monday about how to make start for new DEVs easier, it would be really great to have in dev docs example of “how to write components” but not only in this new style, but also with examples of the deprecated ones (and why we don’t use them anymore)

Right now we have:

  • mapStateToProps (not used anymore)
  • Arrow function component with hooks (won’t used after this new)
  • This new one
  • (… probably some other, not used anymore too)

I can take care about docs update.

3 Likes

How about this?

RegistrationCommandsPage/index.js
RegistrationCommandsPage/styles.scss
RegistrationCommandsPage/actions.js
RegistrationCommandsPage/helpers.js
RegistrationCommandsPage/selectors.js

Do we really need the component name in each file name?

I like the simplicity with the naming,
as for actions.js, reducer.js, selectors.js - they can live in a single file called slice.js if we are going to use redux/toolkit

+1 for the docs

Can you clarify this? Is it being suggested not to use hooks? Not to use arrow functions? (I’d be -1 to either of those in general)

I don’t really care if a hooks component is written const myComponent = props => ... or function myComponent(props) { ... }, if that’s what you mean…

I’m totally +1 to these:

:white_check_mark: Use hooks as much as possible. It’s the way forward.
:white_check_mark: Use react-testing-library instead of snapshots, and focus our tests more on user functionality and less on implementation details
:white_check_mark: In components that use Hooks, no need to have an index.js file just to export the component. This is a remnant of the old container / presentational component pattern and isn’t really needed any more because of the way Hooks makes everything composable.

I’m a bit skeptical of the slice pattern, just because combining actions / selectors / reducers into a single file will result in huge files. Some of our reducers are gigantic… I think if they are in the same folder, it’s really not too much effort to switch between those files.

Likewise I’m not sure about adding the redux-toolkit library, since I’m unfamiliar with it and what benefit it would provide. One thing I’ve always loved about the Foreman project is that we don’t default first to using external libraries, instead we try first to solve things ourselves. (for example, instead of adopting redux sagas or thunk, we wrote our own Redux API middleware which is excellent, and we keep building onto it!)

Once we agree on new code conventions I’m all for enforcing them via eslint. It’s a lot easier than ‘discussing’ it every time on the pull request. :yum:

In components that use Hooks, no need to have an index.js file just to export the component. This is a remnant of the old container / presentational component pattern and isn’t really needed any more because of the way Hooks makes everything composable.

I think that any component should use the index.js file instead of the extra Component.js file.

I’m a bit skeptical of the slice pattern, just because combining actions / selectors / reducers into a single file will result in huge files. Some of our reducers are gigantic… I think if they are in the same folder.

it depends, I think that for most of the component that want to use Redux, the slice.js file won’t be too big, maybe some of this huge files are leftovers of our old approach, even before the API middleware, and could move some of the logic into the index.js with local React state.

it’s really not too much effort to switch between those files.

as per several comments above, it can make reviewers life easier if it’s a single file,
and it can make the PR to get merged faster.

Likewise I’m not sure about adding the redux-toolkit library, since I’m unfamiliar with it and what benefit it would provide. One thing I’ve always loved about the Foreman project is that we don’t default first to using external libraries, instead we try first to solve things ourselves.

That’s true, although when it comes to file conventions, I would follow what most of the community are using, and because redux created this to reduce boilerplate, it sounds like a good idea to follow that,

here is an example where I tried to implement all of the actions, reducer and selector in the same file, which is about 50 lines:

and with Redux slice it will be much smaller, here is an example:

import { createSlice } from '@reduxjs/toolkit'

const initialState = { value: 0 }

const counterSlice = createSlice({
  name: 'counter',
  initialState,
  reducers: {
    increment(state) {
      state.value++
    },
    decrement(state) {
      state.value--
    },
    incrementByAmount(state, action) {
      state.value += action.payload
    },
  },
})

export const { increment, decrement, incrementByAmount } = counterSlice.actions
export default counterSlice.reducer

But I definitely agree that on a very big file we can move it to separate smaller files,
or rethink if we really need all of that functionality - it might be good to move it to local state.

Oh, my mistake, poorly written :confused: I was talking about new folder structure suggested in the RFC (slice.js) Shame I can’t edit the original post.

There is no plan (at least for what I know) to drop arrow function components & hooks, just to re-think the folder structure.

I think this pattern is a good fit for simple cases where the actions and reducers are mostly boiler plate. If we have components that have very complicated reducers/actions, perhaps it would make sense to keep them separate (or consider refactoring the component into smaller, simpler components or to move some of the logic out of the store).
From a reviewer stand point, it is much easier to read and understand a 100 line file than jumping back and forth between two 50 line files. And from the examples at least it would seem this pattern would potentially also reduce the size of the files as much of the boilerplate is eliminated.

Perhaps it would be a good exercise to open a quick POC PR that adds the redux-toolkit library and take one simple component to change to using slice to demonstrate what it would look like and how much simpler it would be?

because the discussion here is still going,
we decided on the Confirm Modal component to write it in the old style,
but I can definitely open a follow-up PR to refactor it to the slice structure as a POC

These two shouldn’t be related IMHO, and i wouldn’t rewrite the modal now to the old style as it isn’t ready for merge yet and this rfc seems close to resolution.
I think if we have a PR that only adds this library and demonstrates how it simplifies one small component it would be easier to come to an agreement on this RFC. I can try to open it myself next week if no one else gets to it sooner.

1 Like

I’ve created a POC of using slices at:
https://github.com/theforeman/foreman/pull/8657

Let’s discuss it tomorrow at the UI SIG and come to an agreement regarding this RFC.

1 Like