machines: follow some react/redux best practices #9461

KKoukiou · 2018-06-21T11:25:57Z

Note: The two first patches of the series are not relevant and PR will be rebased on top of https://github.com//pull/9166 once it will be merged.

Since more contributors are/will be coming to cockpit machines, let's make the package a bit more friendly for new comers by following some of the best practices mentioned in he official docs. (See https://redux.js.org). This will involve unfortunately some code move but IMO it's better to do it now, than wait for more features to be introduced and the project to get bigger.

As a start I posted two patches:

First creating constant for actions types, see why here https://redux.js.org/recipes/reducing-boilerplate#actions
Second splitting the actions.es6 file and sorting the functions in alphabetical order because it was getting really big, and there it was not clear if there was some pattern on where new actions creators should be placed inside the file.

I would like to see some organization in the components directory as well, to make it clear which are Presentational and which Container Components but let's keep it for another PR.

For new contributors it will be helpful as well, if we have clear naming conventions for actions, actions types and selectors.
I appreciate this one:

action name: <NOUN>_<VERB>
action creator name: <verb><Noun>
selector name: get<Noun>

See reasoning here: https://medium.com/@kylpo/redux-best-practices-eef55a20cc72
But I would be ok to keep the current (followed in most cases) which is:

action name: <VERB>_<NOUN>

In any case let's keep it documented.

mareklibra · 2018-06-21T14:18:00Z

Thank you for cleaning this up.
Scope and reasoning seems to be clear, I have no review comments so far.

KKoukiou · 2018-06-22T11:53:20Z

@mareklibra I actually have one question.

So, the actions and action creators are logically divided into two categories,

the store ones ( which are actually going to be handled by reducers to make changes to the store) - - - the provider ones

The provider action creators are actually always resulting in dispatching actions from the store-actions-types, since the store action types are the only ones that have reducers. In addition, all the provider-actions-types are not actually actions since they never appear in any action object.
From the docs:
Actions are plain JavaScript objects. Actions must have a type property that indicates the type of action being performed.
aka look like this:

{
  type: MY_ACTION,
  ...
}

I understand that you wanted somehow to encapsulate all these async operations that these functions are performing, thus you decided to create these fake action-creators for them. But for a new comer to the project, who is not familiar with redux this is really confusing to have in the action-types something that is never used as a function.
So, unless I am getting something wrong 🤔 , maybe

we should move all the provider-actions to other files, to stop suggesting that they are actions.
else we can actually use come middleware like promiseMiddleware, and actually create actions from these functions. Then we can have more fine grained actions, specific to each operation.

WDYT?

mareklibra · 2018-06-26T08:25:16Z

@KKoukiou , I see same differentiation of the actions as you mentioned, but in my understanding the separation is more about whether an action has external effect or just manipulates local data. This results in the same split as described above, but the way of thinking is slightly different.

So I'm fine with splitting these actions to multiple files.

Important point for me is the way how an action is emitted. The consuming code (means React) should "just dispatch an action" if whatever has changed or needs to be done. This is enabled by the thunk middleware which logic/implementation seems super-simple to me.

Right, we can wrap the actions to plain JS data objects as we do for the second type of actions, but I don't think this will bring any benefit for us.
Simple calls are handled via single (and already simple) virt() method. Further, the provider code can just return functions and they are handled same way.

KKoukiou · 2018-06-26T09:05:54Z

@mareklibra thanks for reply. I also added one more commit, which defines helper functions for the action creators. I think it's a good idea in order to force the FSA standard since now half actions are following FSA and half are not. WDYT?

mareklibra · 2018-06-28T10:24:30Z

pkg/machines/actions/store-actions.es6

 */

+function makeActionCreator(type, ...argNames) {
+    return function (...args) {
+        const action = { 'type': type }


nit: const action = { type }

mareklibra

I'm fine with action creator helper functions. No matter I do not see writing of action creator methods as a big deal, since they are quite straightforward to write even without any helper function.
I don't think there's so much boilerplate in writing them "from scratch", the complexity is somewhere else, imo.
And considering KISS, their content is clear at the first glance.

I must admit, that I leverage recent ("from scratch") form while debuging - I can easily add check or log there. This will be more complicated when wrapped. But please note, this is not a blocker, just a note regarding workflow.

In general, the use of non-FSA actions is just a historic left-over in the Cockpit codebase.
IMO, we can start with refactoring of all actions to FSA and not differentiate them.

There should not be much logic within action creators. But I found quite useful to implement some "translation or unification" logic within them occasionaly. Nothing complicated but just making the calls simpler.

Regarding recent implementation:
The contract for FSA and non-FSA is different - the FSA action creator will consume a single object, while non-FSA list of args.
If improvement is needed, I would start with unification of this.

mareklibra · 2018-06-28T10:28:57Z

pkg/machines/actions/store-actions.es6

+function makeActionCreatorFSA(type, ...argNames) {
+    return function (...args) {
+        const action = { 'type': type, 'payload': args[0] }
+        return action


Can be one-line statement

mareklibra · 2018-06-28T10:29:04Z

pkg/machines/actions/store-actions.es6

+
+function makeActionCreatorFSA(type, ...argNames) {
+    return function (...args) {
+        const action = { 'type': type, 'payload': args[0] }


The returned function expects just a single argument. If this would be the way to go, let's make it clear at the first glance.

KKoukiou · 2018-07-18T15:51:39Z

@martinpitt I rebased, please remove needs-rebase label.

martinpitt · 2018-07-23T19:20:19Z

pkg/machines/constants/store-action-types.es6

+ */
+
+// --- Store actions --------------------------------------------
+export const ADD_NOTIFICATION = "ADD_NOTIFICATION";


I can't say I like this very much.. it is yet another place to change when one adds new functionality (but then again with redux this is already ridiculously heavily split), and it makes things harder to debug (as you see numeric values).

But I don't veto this if @mareklibra agrees - at this point the "machines" code is maintained by your team mostly anyway, so you can pick the code style.

@martinpitt I don't get your point by "and it makes things harder to debug (as you see numeric values)." Where you 'll see the numeric values?
I don't really see anything against this split. Benefits can be seen here. https://redux.js.org/recipes/reducing-boilerplate#actions
In general I would like to keep somehow naming consistent, and having all action type in one file makes it easier to review.
But I will not insist.

Where you 'll see the numeric values?

I didn't actually see them, but won't they appear in stack traces and similar views, instead of the string constants?

But as I said, I don't veto this for the machines page, it was just a comment.

KKoukiou · 2018-07-25T14:13:20Z

Getting rid of of last commit for action creator helper functions, reconsidered its usefulness.

Since strings are prone to typos and duplicates this approach helps avoiding errors that will be difficult to debug. Read more about benefits of this approach here: https://redux.js.org/recipes/reducing-boilerplate#actions Signed-off-by: Katerina Koukiou <[email protected]>

actions.es6 file contains both provider and store actions. Let's keep them in seperate files to increase readability and make it easier for new maintainers to navigate through the package. Together with the move the functions were also sorted in aphabetical order. Signed-off-by: Katerina Koukiou <[email protected]>

martinpitt · 2018-07-26T20:11:49Z

I merged this with fixing two missing semicolons (fatal now after #9736), and dropping the Signed-Off-By. Thanks!

KKoukiou force-pushed the cleanup-machines branch from 3f40151 to 91a6b07 Compare June 21, 2018 11:47

mareklibra reviewed Jun 28, 2018

View reviewed changes

martinpitt added the needs-rebase label Jun 29, 2018

KKoukiou force-pushed the cleanup-machines branch from 2e83f7a to ae976f0 Compare June 29, 2018 18:01

KKoukiou force-pushed the cleanup-machines branch from ae976f0 to b62fb3f Compare July 18, 2018 15:48

martinpitt added needs-rebase and removed needs-rebase labels Jul 18, 2018

martinpitt reviewed Jul 23, 2018

View reviewed changes

KKoukiou added 2 commits July 25, 2018 17:12

KKoukiou force-pushed the cleanup-machines branch from b62fb3f to f067d73 Compare July 25, 2018 17:26

martinpitt removed the needs-rebase label Jul 26, 2018

martinpitt closed this in 24ccf2e Jul 26, 2018

KKoukiou deleted the cleanup-machines branch November 29, 2018 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

machines: follow some react/redux best practices #9461

machines: follow some react/redux best practices #9461

KKoukiou commented Jun 21, 2018 •

edited

Loading

mareklibra commented Jun 21, 2018

KKoukiou commented Jun 22, 2018 •

edited

Loading

mareklibra commented Jun 26, 2018

KKoukiou commented Jun 26, 2018

mareklibra Jun 28, 2018

KKoukiou Jun 29, 2018

mareklibra left a comment

mareklibra Jun 28, 2018

KKoukiou Jun 29, 2018

mareklibra Jun 28, 2018

mareklibra Jun 28, 2018

KKoukiou commented Jul 18, 2018

martinpitt Jul 23, 2018

KKoukiou Jul 25, 2018

martinpitt Jul 25, 2018

KKoukiou commented Jul 25, 2018

martinpitt commented Jul 26, 2018

machines: follow some react/redux best practices #9461

machines: follow some react/redux best practices #9461

Conversation

KKoukiou commented Jun 21, 2018 • edited Loading

mareklibra commented Jun 21, 2018

KKoukiou commented Jun 22, 2018 • edited Loading

mareklibra commented Jun 26, 2018

KKoukiou commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKoukiou commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKoukiou commented Jul 25, 2018

martinpitt commented Jul 26, 2018

KKoukiou commented Jun 21, 2018 •

edited

Loading

KKoukiou commented Jun 22, 2018 •

edited

Loading