Show available updates #930
Conversation
dashboard/src/shared/types.ts
Outdated
| } | ||
|
|
||
| export interface IChartUpdatesList { | ||
| name: string; |
There was a problem hiding this comment.
The list of updates do not depend on the version?
There was a problem hiding this comment.
If you mean the latest version it's in the IChartUpdate, if you mean the current version of the chart is not necessary.
| @@ -84,6 +86,16 @@ class AppView extends React.Component<IAppViewProps, IAppViewState> { | |||
| sockets: [], | |||
| }; | |||
|
|
|||
| private getUpdates = _.memoize((app: hapi.release.Release) => { | |||
There was a problem hiding this comment.
Could this not be avoided by moving the call for getChartUpdates to either ChartInfo or AppControls? I know it's confusing because they both use it, but it could be similar to the ServiceItem and AccessURLItem where the AccessURLItem expects ServiceItem to load the Service in the state.
cc @migmartri @Angelmmiguel - would like to get your thoughts on this too
There was a problem hiding this comment.
I am not sure what you want to avoid, the call to memoize or calling to getChartUpdates here when the components that use it are ChartInfo and AppControls?
IMO having a component that relies on other component to update the redux store is not the best approach it may lead to a brittle structure. If the component that updates the store changes and no longer does that there is no way for the other component to catch that and avoid the bug.
There was a problem hiding this comment.
I believe the discussion about calling the method from here or child componentes will depend on how ChartInfo and AppControls are implemented. If they're just components that show information (presentational), they shouldn't call call methods to initialize the information. This is the job of the componentes that contains the logic of the view.
However, I believe the main issue of this code is that it's out of the React component lifecycle. This method is called in the render method, so any update on the component or its branch will trigger a call to getUpdates. _.memoize makes the trick here, but the call it's still present. If this code changes in the future and don't memoize the content it may cause a performance issue in this view.
Retrieving information from external services must be done during the lifecycle of the component. I recommend to do it right after the component is mount (componentDidMount). In this case, this is not possible because this call depends on another API call. So, you can use componentDidUpdate instead. You only need to check the value it's not present yet and it's not loading at that point. The second consideration it's very important to avoid calling getChartUpdates several times.
Just to clarify why this is important, all the modification of the state and properties that affects to the component must be known by React. Extracting this logic to internal variables of the class may cause performance issues or wrong updates.
Another issue I found is the import of lodash. I've opened a second task for it: #939.
There was a problem hiding this comment.
Thanks so much for the detailed feedback @Angelmmiguel! @andresmgot I do agree that having dependent components is troublesome, however in this case it might be possible for both components to kick off the call to getChartUpdates and the action can deal with avoiding fetching again if we already have it (there could be a race condition, but maybe it's not a huge issue to fetch it twice).
Anyway, I think we can keep this in AppView for now, I'm just concerned that AppView is doing a lot and there must be ways we can break it down and also rely more on componentDidMount in child components rather than utilising componentDidUpdate which comes with the issue of ensuring guards to not fetch data too much.
There was a problem hiding this comment.
An alternative we haven't considered is to dispatch getChartUpdates from getApp to avoid the need for waiting for the app prop to be set in the component.
There was a problem hiding this comment.
An alternative we haven't considered is to dispatch getChartUpdates from getApp to avoid the need for waiting for the app prop to be set in the component.
I am reluctant to change the approach again. That should be valid as well but we will be mixing chart actions with app actions, which may makes sense if we move the updates information to the release object but since this is working and there is no a major issue let's go for this. If we find a different issue we can re-evaluate but for the moment I think it should be fine.
There was a problem hiding this comment.
Yeah I think it's just something we should consider for the future. I'll be honest, I'm not a big fan of using componentDidUpdate just for this thing, and continue using componentWillReceiveProps for everything else. It might have been better to convert the whole component to use componentWillReceiveProps. Nevertheless, I don't think it's worth reverting that now, so I'm fine merging this.
| @@ -84,6 +86,16 @@ class AppView extends React.Component<IAppViewProps, IAppViewState> { | |||
| sockets: [], | |||
| }; | |||
|
|
|||
| private getUpdates = _.memoize((app: hapi.release.Release) => { | |||
There was a problem hiding this comment.
I believe the discussion about calling the method from here or child componentes will depend on how ChartInfo and AppControls are implemented. If they're just components that show information (presentational), they shouldn't call call methods to initialize the information. This is the job of the componentes that contains the logic of the view.
However, I believe the main issue of this code is that it's out of the React component lifecycle. This method is called in the render method, so any update on the component or its branch will trigger a call to getUpdates. _.memoize makes the trick here, but the call it's still present. If this code changes in the future and don't memoize the content it may cause a performance issue in this view.
Retrieving information from external services must be done during the lifecycle of the component. I recommend to do it right after the component is mount (componentDidMount). In this case, this is not possible because this call depends on another API call. So, you can use componentDidUpdate instead. You only need to check the value it's not present yet and it's not loading at that point. The second consideration it's very important to avoid calling getChartUpdates several times.
Just to clarify why this is important, all the modification of the state and properties that affects to the component must be known by React. Extracting this logic to internal variables of the class may cause performance issues or wrong updates.
Another issue I found is the import of lodash. I've opened a second task for it: #939.
Ref: #588
Requires: helm/monocular#592
Show in the App view when there is an update available. If that's the case a message in the ChartInfo box is shown and the "Upgrade" button gets highlighted:
If the mouse is over the "Upgrade" button it also shows a message with the new version found:
If the application is up to date, only a message is shown in the ChartInfo box and the upgrade button is rendered as always: