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 app details #25

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

solomon23
Copy link
Contributor

@solomon23 solomon23 commented Sep 17, 2019

Hello !

I was digging into the the frontend code to mockup a feature and noticed there were some improvements that could be made. The biggest issue is the app doesn't use a unidirectional flux like data flow and many of the classes do too much work.

WHAT IS THIS

I've done a refactor of the app details pages. During the refactor a few bugs were fixed here and there but there were a ton of code wins that will help the app moving forward.

WHAT ARE THE WINS?

  1. There is now an App Details Provider which sits in the new React context and is the source of data "truth". All actions go through the provider and it holds the state for pages beneath. This is a lightweight way of building a flux pattern instead of Redux or other solution. We also get wins like whenever an action is fired we can change the loading state and bubble it down. Any changes to app def are stored in the state. This removes all the props passing up and down the chain which should be avoided.

We can also do nice things like add a dirty flag here since all things updated in the same place. Then notify users if they navigate away without doing a save. Also when we do any actions we know the current state and react to it very cleanly.

  1. Moved the Log Polling into its own service. Now it doesn't live with the component that might need it but there's a global one for all the pages. This makes it easier to coordinate them and maybe combine them all into one call and then spread them down to the components. I've built ( but not part of this pull ) an endpoint that gets the container states and it fits nicely into this service. You also get good insight into the lifecycle of this component.

  2. A lot the classes were very large. I've pulled things out into more logical components for easier maintenance and scoping. This will be much nicer when it's time to add client side unit tests.

  3. No more ! I've typed everything ( well there's 1 left... )

  4. No more "!". All the objects are accessed in a "safe" way.

  5. Adding ESLinting. I've just added linting for my new files. I'm using @typescript-eslint/recommended and react/recommended. Plus a few rules that seem to follow how you already write code ( double quotes, semicolons, ident, no explicit return type ). All passing super clean.

  6. Added a common Log Scrolling view with some nice fixes

  7. Integrated the loading state for saving so it doesn't destroy and recreate the tabs.

  8. Changing tabs now causes a navigation so you can refresh the page or link to a tab

This includes:
Using a unidirectional data flow using context
Adding a top levfel provider which changes the state and performs api actions
Pulling out files into components to make them more maintainable
Adding a service which takes care of the server ping'ing instead of doing it in the ui comonent
cleanup the context not being nullable
add back the redirect to apps if the app can't be found
clean up a bunch of common linting rules
add rules for quotes and semicolons that are used through the site
add trailing commas
add a safer deep copy with support for undefined
add an error message for bad json
This lets us remove the componentWillReceiveProps and replace it with componentDidUpdate and remove our react 17 warning
…self

pull log views out of applogs and build logs and use a common log
LiquidITGuy pushed a commit to LiquidITGuy/caprover-frontend that referenced this pull request Feb 17, 2023
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.

1 participant