Open catherineluse opened 2 years ago
Marking as size/20 due to the huge amount of code in shell/config/product
that would be affected if all the imperative config code there was made declarative.
getGroups is covered by https://github.com/rancher/dashboard/issues/7773
Reason 1: Performance
When the Cluster Explorer loads for the first time, it creates over 300 mutations to the store.
This is an antipattern because we should only use the store to cache data from the back end, or to track changes that affect multiple components that are not close to each other in the DOM. Most state in Vue should be handled within individual components or with events. And this side nav code doesn't even have to do with state - it is just about loading a single component for the first time.
In principle, a single component shouldn't have to mutate the store at all when it loads for the first time. All the logic for building the side nav should be handled within the side nav component itself.
So if this is refactored to avoid mutating the store, it may be faster to load Cluster Explorer for the first time. When I say faster, I mean we could first show a default (mostly empty or with a loading spinner) side nav so the user does't stare at a totally empty screen, then modify it after the schemas are loaded, then modify it again after we do any async calls that we need to do to figure out whether to show links to certain services deployed from helm charts.
Reason 2: Ease of use
I struggle with
type-map.js
. Here are the two tasks I struggled with recently becausetype-map.js
was so hard to read:We will have to update this side nav many times in the future. The question is, should it be easy to update, or should it be hard?
Reason 3: Would require a refactor to ever be in a component library
Because the side nav has so many dependencies on Vuex, that means it couldn't be used by other applications. Anything that goes in the component library would have to be self-contained.
Reason 4: We need to allow async calls to affect what is displayed
Currently the code is imperative and is strictly required to run after the schemas are loaded, but before anything else is.
This means we can't wait to show someone a link to a service such as NeuVector in the side nav until we make an async call to ensure the service exists. So we just hardcode a link and assume the service will be there. This leads to a bad UX when the user clicks the link but it's broken because the service is not deployed.
Reason 5: Customization
Kenneth said customers gave feedback that they wish they could fully customize what is seen in the side nav so they can limit their users to a UI that is more focused toward their purpose. The current implementation doesn't allow for a very detailed level of customization.
Objective
If this code is refactored, the goal should be:
type-map.js
for hours.config
folder should be declarative instead of imperative. For example, that means instead of callingconfigureType
three times in a row (which mutates the store three times), we should define the configuration in an object.type-map.js
should never mutate the store. Because again, the real purpose is for configuration, not altering state.Method
Here's what I did to get to the "over 300" number.
Added logs to the parts of
type-map.js
that mutate the store:Results of loading the side nav for the first time:
Example Config
This is an example output configuration resulting from the >300 mutations. I wish it was easier to predict what this final output looks like.