Refactor modifications in upgrade and deployment forms #1283

andresmgot · 2019-11-11T16:37:34Z

Follow up of #1278

This refactor:

Adds a new property to the IChartState: deployed. Instead of using selected and taking advantage that the first version selected is the one deployed, now we explicitly store that chart version in a different variable. This requires changes in the Redux store but allows us to pre-select the latest version in the upgrade form (not included in this PR)
Move the logic to manage modifications outside the BasicDeploymentFormBody. This procedure is specific to the upgrade flow so it's now in the UpgradeForm component.
Since the process of changing the appValues when changing the version is different between the deployment and the upgrade form, that's been extracted from the BasicDeploymentFormBody as well. In the case of the deployment, we simply apply the default values of the new version while in the upgrade scenario we also apply modifications if they exist.

absoludity

Some small points and one big one: Can we please update past react 16.2 before doing this PR, so that we can use getDerivedStateFromProps and stop adding componentWillReceiveProps methods (which will need to be removed with effort very soon) or at least have to prefix them with UNSAFE_ and consider how we can avoid them.

absoludity · 2019-11-13T05:49:37Z

dashboard/src/actions/charts.ts

@@ -25,6 +25,13 @@ export const selectChartVersion = createAction("SELECT_CHART_VERSION", resolve =
    resolve({ chartVersion, values, schema });
 });

+export const requestedDeployedChartversion = createAction("SELECTED_DEPLOYED_CHART_VERSION");


Why the different name from the string value here? (ie. why not selectedDeployedChartVersion, matching the string action value, or updating the string value to "REQUESTED_..." to match)

Also, we really should have some standard here... I see requestCharts/receiveCharts (which makes sense as a pattern), while for another action it is selectChartVersions / receiveChartVersions (select doesn't really make sense here, I think it's request?). Anyway, for this new one, can we do requestDeployedChartVersion (ie. no ed) and receiveDeployedChartVersion (ie. matching requestCharts/receiveCharts) and possibly update the other action here (unless I'm missing why select is being used).

you are right, I didn't properly read these

absoludity · 2019-11-13T06:02:35Z

dashboard/src/components/AppUpgrade/AppUpgrade.test.tsx

+    expect(wrapper.state("repo")).toEqual(repo);
+  });
+
+  it("should request the deployed chart when teh app and repo are populated", () => {


Tiny typo - s/teh/the/

absoludity · 2019-11-13T06:06:46Z

dashboard/src/actions/charts.ts

+
+export function getDeployedChartVersion(
+  id: string,
+  version: string,


Is there some confusion here about a chart version vs a chart revision, or are we defining a function called getDeployedChartVersion which takes the chart version as input? confused

yeah, it's a bit confusing but it's the name pattern chosen here. The function needs a version as a string and returns a IChartVersion which is all the information about that version. The difference with a IChart is that IChart contains a reference to the latest version instead of a specific version.

absoludity · 2019-11-13T06:13:52Z

dashboard/src/components/AppUpgrade/AppUpgrade.tsx

  public componentDidMount() {
    const { releaseName, getAppWithUpdateInfo, namespace, fetchRepositories } = this.props;
    getAppWithUpdateInfo(releaseName, namespace);
    fetchRepositories();
  }

+  public componentWillReceiveProps(nextProps: IAppUpgradeProps) {


We seem to keep adding more componentWillReceiveProps when we should be removing them as we go, not adding?

See https://reactjs.org/docs/react-component.html#updating

The issue here is that we're adding more code that is dependent on detecting a change in the prop values, rather than working towards the functional getDerivedStateFromProps. Even if we can't use that here (it's added in 16.3), let's not add code which we're going to have to remove/update in the future? We should be considering the reasons why react is removing componentWillReceiveProps (ie. so that people won't be deriving state based on existing state or old prop values).

I completely forgot that componentWillReceiveProps was the one being deprecated. In this case, I think it's okay to use componentDidUpdate. I am reluctant to use getDerivedStateFromProps since it's a bit complex (and we need to use componentDidUpdate anyway for the second part of this function)

absoludity · 2019-11-13T06:25:54Z

dashboard/src/components/DeploymentForm/DeploymentForm.tsx

+    if (nextProps.selected.version !== this.props.selected.version && !this.state.valuesModified) {
+      this.setState({ appValues: nextProps.selected.values || "" });
+    }
+  }


Again, this looks like we're adding more code which calculates state by detecting changes in props, rather than simply a function of the state... I worry we're going to pay for this later when componentWillReceiveProps is no longer supported (it'll be removed in 1.17).

Can we first update to React 16.3 (at least, but possibly latest - 16.11), before we add more code like this? At least then we'll be able to use getDerivedStateFromProps and will be forced to add UNSAFE_ to these unsafe methods we're adding?

the same here, it's true that we should not be using componentWillReceiveProps but I prefer using componentDidUpdate because the way to use it is more similar

Sounds good.

absoludity · 2019-11-13T06:38:24Z

dashboard/src/components/UpgradeForm/UpgradeForm.tsx

@@ -41,6 +46,28 @@ class UpgradeForm extends React.Component<IUpgradeFormProps, IUpgradeFormState>
    valuesModified: false,
  };

+  public componentWillReceiveProps(nextProps: IUpgradeFormProps) {


Ditto for getDerivedStateFromProps

andresmgot

Hi @absoludity, thanks for the review. It's true that we should not be using componentWillReceiveProps but I'd rather use componentDidUpdate which is the closest one. Let me know what you think.

andresmgot · 2019-11-13T10:40:51Z

dashboard/src/actions/charts.ts

@@ -25,6 +25,13 @@ export const selectChartVersion = createAction("SELECT_CHART_VERSION", resolve =
    resolve({ chartVersion, values, schema });
 });

+export const requestedDeployedChartversion = createAction("SELECTED_DEPLOYED_CHART_VERSION");


you are right, I didn't properly read these

andresmgot · 2019-11-13T10:44:27Z

dashboard/src/actions/charts.ts

+
+export function getDeployedChartVersion(
+  id: string,
+  version: string,


yeah, it's a bit confusing but it's the name pattern chosen here. The function needs a version as a string and returns a IChartVersion which is all the information about that version. The difference with a IChart is that IChart contains a reference to the latest version instead of a specific version.

andresmgot · 2019-11-13T10:48:11Z

dashboard/src/components/AppUpgrade/AppUpgrade.tsx

  public componentDidMount() {
    const { releaseName, getAppWithUpdateInfo, namespace, fetchRepositories } = this.props;
    getAppWithUpdateInfo(releaseName, namespace);
    fetchRepositories();
  }

+  public componentWillReceiveProps(nextProps: IAppUpgradeProps) {


I completely forgot that componentWillReceiveProps was the one being deprecated. In this case, I think it's okay to use componentDidUpdate. I am reluctant to use getDerivedStateFromProps since it's a bit complex (and we need to use componentDidUpdate anyway for the second part of this function)

andresmgot · 2019-11-13T10:54:43Z

dashboard/src/components/DeploymentForm/DeploymentForm.tsx

+    if (nextProps.selected.version !== this.props.selected.version && !this.state.valuesModified) {
+      this.setState({ appValues: nextProps.selected.values || "" });
+    }
+  }


the same here, it's true that we should not be using componentWillReceiveProps but I prefer using componentDidUpdate because the way to use it is more similar

absoludity

Great, thanks for the update Andres.

absoludity · 2019-11-14T05:58:07Z

dashboard/src/components/AppUpgrade/AppUpgrade.tsx

+      repo = this.props.repo;
+      if (repo && repo.metadata) {
+        // If the repository comes from the properties, use it
+        this.setState({ repo });


Why are we storing props in the state like this? There should be no need if it's available in the props? Ah - I see below because it may not be in the props and may need to be fetched... so the state is guaranteed to have the repo?

Exactly, the repo will be only available in the props when using the SelectRepoForm if not, it's obtained from the app info. For simplicity, I stored it in the state.

absoludity · 2019-11-14T06:00:05Z

dashboard/src/components/AppUpgrade/AppUpgrade.tsx

+            this.setState({ repo: repoWithLatest });
+            repo = repoWithLatest;
+            this.setState({ repo });


Looks like 2 of these lines are redundant?

absoludity · 2019-11-14T06:01:59Z

dashboard/src/components/DeploymentForm/DeploymentForm.tsx

+    if (nextProps.selected.version !== this.props.selected.version && !this.state.valuesModified) {
+      this.setState({ appValues: nextProps.selected.values || "" });
+    }
+  }


Sounds good.

Refactor modifications in upgrade

c8585ea

andresmgot requested a review from absoludity November 11, 2019 16:37

andresmgot mentioned this pull request Nov 12, 2019

Select the latest version when upgrading #1284

Merged

absoludity reviewed Nov 13, 2019

View reviewed changes

Apply review

c443685

andresmgot commented Nov 13, 2019

View reviewed changes

absoludity approved these changes Nov 14, 2019

View reviewed changes

Andres Martinez Gotor and others added 2 commits November 14, 2019 11:18

Remove redundant code

ca03a85

Merge branch 'master' into modificationsRefactor

53529e1

andresmgot merged commit a10f05c into vmware-tanzu:master Nov 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor modifications in upgrade and deployment forms #1283

Refactor modifications in upgrade and deployment forms #1283

andresmgot commented Nov 11, 2019

absoludity left a comment

absoludity Nov 13, 2019

andresmgot Nov 13, 2019

absoludity Nov 13, 2019

absoludity Nov 13, 2019

andresmgot Nov 13, 2019 •

edited

Loading

absoludity Nov 13, 2019

andresmgot Nov 13, 2019

absoludity Nov 13, 2019

andresmgot Nov 13, 2019

absoludity Nov 14, 2019

absoludity Nov 13, 2019

andresmgot left a comment

andresmgot Nov 13, 2019

andresmgot Nov 13, 2019 •

edited

Loading

andresmgot Nov 13, 2019

andresmgot Nov 13, 2019

absoludity left a comment

absoludity Nov 14, 2019

andresmgot Nov 14, 2019

absoludity Nov 14, 2019

andresmgot Nov 14, 2019

absoludity Nov 14, 2019

Refactor modifications in upgrade and deployment forms #1283

Refactor modifications in upgrade and deployment forms #1283

Conversation

andresmgot commented Nov 11, 2019

absoludity 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

andresmgot Nov 13, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absoludity 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

Choose a reason for hiding this comment

andresmgot Nov 13, 2019 •

edited

Loading

andresmgot Nov 13, 2019 •

edited

Loading