Rethinking React-Redux folder structure

Development RFCs
5

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
6

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.

7

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.

8

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

9

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

10

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
11

:+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
12

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

13

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…

14

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:

15

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.

16

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.

17

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?

18

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

19

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
20

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
21

Awesome work @tbrisker! thanks for doing it :slight_smile:
opened also feat(vendor-core): add @reduxjs/toolkit library by laviro · Pull Request #319 · theforeman/foreman-js · GitHub

22

This RFC was discussed again today on the UX Interest group meeting and the remaining concerns were addressed. Unless anyone raises any further concerns I believe we can consider this accepted. Next steps would be:

merge @Ron_Lavi’s PR adding the redux toolkit library in to foreman-js
releasing a new version of @theforeman/vendor - 8.8.0 released
update packaging to use newer @theforeman/vendor - Bump foreman-js by tbrisker · Pull Request #6873 · theforeman/foreman-packaging · GitHub
update docs to recommend the new structure
profit!

Who is taking care of which task?

23

I can write the docs

24

The updated vendor package has been released and I’ve turned my POC PR into a real one that also requires the newer vendor version and implements the new folder structure.

2 Likes