Foreman and Katello's Javascript Ecosystem - What the Flux?

Development
1

Ok first, big disclaimer here, I’m arriving on the scene quite late, and I know that JS in Foreman/Katello has a long and winding history. Also, I’m fairly new to React and Redux but have been reading up and attending talks/events around it. Probably lots of these ideas are coming from Thinking in Redux. Please correct me if I’m misinformed about anything.

I noticed some concerning things about our Javascript, namely the new React and Redux parts:

  1. Thunk - this is a pretty big issue IMO. It goes against the design patterns Redux is based on, namely event sourcing and Command Query Responsibility Segregation (CQRS).

** In short, event sourcing means that the sequence of events are the golden record, and the state doesn’t matter (think balance = sum(transactions) if you have an issue with your balance, your bank will look through your transaction history to see if the balance is correct).

** CQRS aims to separate completely the parts of code that write state from the things that read the state.

Thunk is redux middleware that allows redux actions to return functions, allowing actions to do ‘stuff’ other than logging their action type and triggering other actions (I know it sounds like actions should do something, but think of them more as events and that ‘something happened’). If we want to react to actions, it’s clear from the redux docs that we should be using middleware (not actions, reducers, selectors, or any other flux pattern files) (aside: Redux is based on the Flux pattern). Of course, Thunk is popular, but that doesn’t mean it’s a Good Thing ;).

  1. Katello doesn’t follow Foreman’s structure; it doesn’t ensure storybooks are supported, despite using the stories directory to put all the stuff.

2a. Katello doesn’t put all the actions together, heck we don’t even put all the redux stuff together. It’s just in there with all the react components, faux middleware logic (not actually registered as middleware in redux). I like how Foreman structures their react_app and redux directories because it’s similar to the way that most other projects seem to organize stuff. (i.e. actions/, reducers/, selectors/, middlware/{core,feature}/, etc.).

  1. Foreman has JS tests in the same directories as their code! I actually don’t mind the tests being ‘near’ the code, but in Katello we at least put the tests in a __test__/ directory inside each JS source directory. It’s frustrating to use Ctrl-P or IDE/GitHub fuzzy search because of this (am I the only one?).

  2. Where are our selectors? This is a really useful part of redux that allows you to format api responses (which are generic and shouldn’t be globally formatted in actions or reducers) for specific pages (i.e. subscriptions product content needs to get info from the subscriptions actions and the products actions).

  3. Action names are strings! And they’re really only there for us, the developers. We should be using that to our advantage and put useful information, such as the resource in question. Here’s an example from Thinking in Redux.

I think I’ll stop here. These are some of my initial concerns. I’m kind of wondering a bit about how all this came to be, whether there were design discussions, agreed-upon patterns to follow, etc. and if there are any documents from back then. Javascript is a unique language in that it allows developers to do whatever they really want without widely accepted best practices (foot.shoot()), and that makes it a difficult language IMO. There are some super old school design patterns that have stood the test of time for good reason. More frameworks isn’t a good solution to most problems versus applying a known pattern, and some frameworks are mostly a pattern more than a framework (redux! :D). Worse, using a framework outside of its designed patterns adds complexity for little to no benefit.

Ultimately, we all have to be on the same page; new contributors are mostly copying what’s currently in the project, so let’s let them have nice, thoughtfully designed code. I’d also love to hear any JS concerns you have that I didn’t touch on. :cat:

1 Like
2

Agree! Providing examples, especially for plugin writers, with “best practice” patterns is going to be key in converting many of the existing pages. Not doing so will just be a source of endless tech debt.

1 Like
3

Overall +1 to making some changes in Katello React/Redux code and keeping patterns in-line with Foreman. Thanks for bringing up this discussion.

Your thinking seems to be in line with the comments in this issue and what Dan Abramov (user gaeron in this post) suggests. I always did think returning API calls in the actions was a little strange. Actions shouldn’t actually do anything except specify what to dispatch to the reducer, which also shouldn’t do anything except modify the state! So probably the best bet is to use middleware even if its just our API wrapper.

As far as directory structure, I am in favor of doing away with scenes and having a components folder and redux folder with the subdirectories you mention above. We can probably flesh this out more if its agreed upon and someone volunteers to do it, but a general +1 to separating redux logic and component logic, also lets keep close to what Foreman does. I’m not too particular about where tests live, so I leave that decision to others :slight_smile:

We can plan the most perfect React refactoring in the world, but someone still has to take the time to actually do it. Maybe it would be helpful to make a list of changes we would like to see, prioritize them, and give a rough difficulty estimate. This would help us plan the changes over upcoming releases and discuss them further. I’m not saying we should do this right now, but maybe after more discussion. Also, specific examples or small diffs of changes you would like to see would be helpful.

And +1 to a Foreman React/Redux best practices/conventions guide for Foreman! This would be great.

Great topic!

1 Like
4

Just a quick note about using redux-thunk: If it’s not already used, I’d rather look into using redux-saga, as it’s IMHO much more capable to handle multiple async tasks, especially with promises (e.g. when using axios and having multiple requests of the same origin because of quick user interaction). The only “downside” is, that one needs to understand how JavaScript generators work.

5

Too late :frowning: See issue #1 of my original post.

1 Like
6

@akofink I did a little more reading into this. I’m going to link to Dan Abramov again (he is the author of Redux after all), he has a great stack overflow response on thunk middleware and when it should be used. This to me suggests that it is an acknowledged middleware for Redux. I don’t think its fair to say it goes against the design pattern Redux is based on, I think it just introduces a new pattern. (I realize I’m contradicting my previous thoughts here, sort of fleshing this out in my brain so excuse any flip-flopping).

But regardless of that, I do think you bring up a great point - are we over-using thunk? We use Redux mostly for API calls, at least on the katello side of things. Do we need thunk to make these calls or is there a pattern we can use that just uses Redux? (genuinely asking) And if so, is it worth the time to switch? Maybe someone who is more experienced in Redux can shed some light on this.

For things like notifications, middleware may make more sense, and redux-saga really does look like a great option instead of thunk, it may fit in our application nicely. I haven’t actually used it though.

Thunk discussions aside - I think cleaning up our directory structure, making sure we transfer components in our move_to_foo directories to the appropriate library, making sure we aren’t duplicating components from foreman, and improving our test coverage should be addressed first.

On a positive note, I have seen a lot of great changes lately to improve and refactor Katello’s React code, so we are already moving in the right direction!

7

I’m afraid I don’t understand how redux-thunk breaks principles of Redux. If you don’t use getState (and we don’t do that AFAIK) it should be fine. What parts of our code do you find in contradiction with what Dan Abramov recommends? I’m aware only about enabling/disabling RH repos where we trigger the api calls directly from a component.

There’s definitely some boilerplate that we can reduce by custom middleware and helpers. But I’d say that’s normal evolution.

The directory structure has been discussed here: Suggestion: Refactor the react_app folder structure People preferred keeping actions and reducers in the same folder as the components. I personally find it more convenient too as I can directly see all the related React and Redux bits at one place.

After the dir structure discussion Foreman started migrating towards this pattern. New code in Katello has been written in this manner too. At the moment the plan is to get rid of the redux/{actions,reducers} folders and of course move the code from move_to_* folders to the appropriate projects. That’s in progress but it’s going to be longer process.

I recently sent a PR that adds basic UI devel docs into Foreman’s storybook: https://github.com/theforeman/foreman/pull/5883
It’s a good place to put all patterns and standards we agree on. The plan is to extend it with further information in future.

8

What parts of our code do you find in contradiction with what Dan Abramov recommends?

@Tomas_Strachota This is what I was saying in my second post, we don’t break any design principles by using thunk, its an acknowledged pattern. I think we agree on this point :slight_smile: sorry for the confusion, was working this out in my head so it probably wasn’t clear.

Happy to hear about the ongoing progress with the directory clean up and the storybook, I’m available to help if needed

9

I guess I got this idea that thunk is Bad from Thinking in Redux, which it sounds like subscribes to the SAM pattern, mentioned in the same stackoverflow post as solving the same problems that thunk does (so many patterns!). I’ve also heard this viewpoint from some redux engineers I’ve met. I’m not suggesting we rip it out because that’d probably be a bunch of work for no benefit, and since there are people here who seem to know about and like thunk, there must be some benefit to it. Again, I’m super new to all this stuff. :slight_smile:

1 Like
10

It’s not clear to me how to add middleware from a plugin to Foreman (i.e. Katello). Am I missing something?