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

Refactor/variables-redux #157

Merged
merged 14 commits into from
Feb 10, 2023
Merged

Refactor/variables-redux #157

merged 14 commits into from
Feb 10, 2023

Conversation

PaulFarault
Copy link
Contributor

@PaulFarault PaulFarault commented Jan 25, 2023

Which issue(s) this PR fixes

Fixes #138

Additional comments

Handle service and component variables through Redux:

  • Initialize Redux with 3 slices:
    • config, to handle user config (read from the config.json in dev mode, and fetched from /config when exported)
    • variables, to handle variables fetched from the server with a loading state.
    • userInput, to hold variables changed by an user for a given service (and its components). Thought, I'm not sure that Redux is the good place to store those infos.
  • TODO: Actualize sub-part of the slice when navigating through services and components. For now, variables are only fetched all at once when loading the app.
  • TODO: Store the logs in a log slice.

This PR allows:

  • To use Redux to handle the global state instead of using useContext/useReducer. Redux is well documented and should be easier to maintain.
  • To fix most of the bug linked to component and service switches.
  • To improve performances, as variables as store since the first API call.

Agreements

@PaulFarault PaulFarault force-pushed the refactor/variables-redux branch 4 times, most recently from 339c478 to 22fc994 Compare February 1, 2023 15:58
@PaulFarault PaulFarault marked this pull request as ready for review February 1, 2023 16:49
@mdrutel
Copy link
Contributor

mdrutel commented Feb 2, 2023

Hi,
I have 2 comments, I don't know if they are bound to this issue ^^'

  1. When you first go to a Service page (eg. : http://localhost:3000/services/hive), when you click on "Validate" without doing anything else, a notification appears ("Variables changed"), but no request is sent to the server.

  2. When you modify a variable and click on "Validate" :

  • first click : the notification "Variables changed" appears
  • second click and next : 2 notifications appears : "Variables changed" and "validating these changes would produce no difference"

@PaulFarault
Copy link
Contributor Author

Thanks for your review, I'll have a look at it!

@PaulFarault
Copy link
Contributor Author

You were right, I added both:

  • a toast / early return statement if nothing have been modified
  • a cleanup of the state when the request succeeded

Would be even better after #131

@PaulFarault
Copy link
Contributor Author

One more thing to fix : When editing a value and then undoing the changes (the store holds the variable already on the server), an error is thrown in addition to this toast :

validating these changes would produce no difference

@PaulFarault
Copy link
Contributor Author

Done, also added a reset of the user inputs when switching from a service to another which was causing a bug.


Thought, I'm not sure that Redux is the good place to store those infos.

This kinda confirms what I thought. Ideally, we should compare the inputted values to the server ones to avoid the "validating these changes would produce no difference" error. However, 2 slices cannot be mixed (variables and userInput in this case).

The solution would be to store in a context local to the service the variables modified for a service and its components. Then, possibly persist this context in the global store (or local storage) if we want to keep the user's uncommitted modifications in memory.

I'm gonna try to roll back to a local context for this feature but this time using a reducer instead of a simple state (keeping the same logic as the slice).

@mdrutel
Copy link
Contributor

mdrutel commented Feb 3, 2023

Hi,

I have other comments ^^'

I suppose that the type of the variable is checked before validation.

  • There's a difference of behavior between the first and the second modification in case of an error : eg. :

    • "hdfs"zz and click on "Validate" => information with "No variables to change"
    • rollback and click on "Validate" => error with "validating these changes would produce no difference"
    • "hdfs"zz and click on "Validate" => error with "validating these changes would produce no difference"
  • With the boolean type, is there a difference between true and "true" ?
    in the YAML file, the result is :

    • true => true
    • "true" => 'true'

Copy link

@mehdibn mehdibn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved from a fonctional point of view

Copy link
Contributor

@mdrutel mdrutel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved from a fonctional point of view

@PaulFarault
Copy link
Contributor Author

PaulFarault commented Feb 10, 2023

@mdrutel

There's a difference of behavior between the first and the second modification in case of an error

The behavior that you describe is a bit odd but is not a big error. In fact, variables that are similar to the one that are one the server shouldn't even be sent. I'll open an issue to improve this later. (#164)

With the boolean type, is there a difference between true and "true" ?

Some Hadoop services ask for boolean, and other for string. It's not really an UI issue. What we could do to improve the ux is:

  • Display the type of each variable in an explicit way. (Variable display styles #130)
  • Use the json schema to show descriptions for those variables.

@PaulFarault PaulFarault merged commit c56cd66 into master Feb 10, 2023
@PaulFarault PaulFarault deleted the refactor/variables-redux branch February 10, 2023 09:26
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 this pull request may close these issues.

Centralize state management
3 participants