Firehose - store firehoses in state to trigger rerender when changed

[rawagner]

When we change resources to fetch, Firehose will restart the watch in componentDidUpdate which is incorrect lifecycle method - this.firehoses are updated after the rerender but we want them to update before.

I've moved the whole logic from componentDidUpdate to shouldComponentUpdate to restart the watch before rerendering.

@spadgett

[rawagner]

another solution would be to use UNSAFE_componentWillUpdate but that is going away soon so I opted for shouldComponentUpdate

[spadgett]

I don't think it's a good practice for shouldComponentUpdate to have side effects. Are you seeing a specific problem? Since the watch is asynchronous, I'm not sure having it in componentDidUpdate is a problem. The render will happen first regardless.

/hold
/cc @christianvogt

[rawagner]

Yes I see the specific problem. I debugged it and here's the flow which leads to problem:

Imagine you have two components - A and B. A is watching some resource (lets say ClusterVersion). B gets mounted and at first isn't watching anything (it is wrapped in Firehose but resources props is empty array). B's Firehoses props from redux are k8sModels - some immutable map, loaded is true and inFlight is false. B will update Firehose's resources props to wach ClusterVersion too, Firehose's render is called but this.firehoses is still empty as that is updated in start/clear functions which weren't called yet.
Sice render was called, componentDidUpdate follows - it will call clear and start which will update this.firehoses. This however wont trigger rerender as state didnt change, props didnt change and props from redux connect didnt change because k8sModels are still the same, loaded is still true (as the resource is already loaded by component B), inFlight is still false. And you end up with Firehose never injecting ClusterVersion to component B, unless there's is some rerender caused by something else.

Specific scenario:
This is actually happening to me in Dashboards page in this PR #2806 .You can easily try it out - first load of dashboards page is ok, Cluster health in Status card is shown fine, then go to some other page (ie pods) and go back to dashboards - Cluster health will be stuck in loading state. This is because masthead-toolbar is watching ClusterVersion (component A from example below) and Status card Cluster health is also watching ClusterVersion (component B). You may wonder why the first render is ok - this is because Firehose's k8sModels, inFlight gets updated so it will trigger rerender, but when changing pages wont as k8sModels are already loaded

[rawagner]

Ok, i agree that shouldComponentUpdate is not a best place to do clear/start. Should I go with UNSAFE_componentWillUpdate ?

[rawagner]

PR updated, i've put this.firehoses to state and also converted UNSAFE_componentWillMount to componentDidMount

[spadgett]

Thanks, this seems like a much better approach, although it looks like it's broken something. e2e-aws-console and e2e-aws-console-olm both failed with the same error.

[rawagner]

/retest

[rawagner]

/test e2e-aws-console

[spadgett]

Are we sure it's safe to remove forceUpdate?

@TheRealJon If you remember the scenario that required forceUpdate, can you check if it's still an issue?

[rawagner]

not sure yet, but it would be nice. A test case would be great, but if you wont remember any, maybe we could try it out and revert if anything goes wrong ?

[TheRealJon]

@spadgett @rawagner I tested this branch out and the project overview workloads page seems to be working fine. I remember adding the forceUpdate prop to firehose because when passing props directly to a firehose child, updating those props did not always trigger a rerender. I don't remember the exact scenario that produced the issue, but it seems like everything is working fine with these updates.

[spadgett]

I remember adding the forceUpdate prop to firehose because when passing props directly to a firehose child, updating those props did not always trigger a rerender.

Oh, so in this example...

https://github.com/openshift/console/blob/master/frontend/public/components/overview/index.tsx#L1186-L1194

If a prop like selectedItem changed, OverviewMainContent didn't re-render since shouldComponentUpdate in Firehose returned false? We should be able to test that.

[TheRealJon]

@spadgett Yes, that's a good example. I did manually test that specific prop, and changing the selected item does work as expected.

[rawagner]

Thanks @TheRealJon . I will add a test to this PR too

[rawagner]

/test e2e-aws-console-olm

[rawagner]

@spadgett added a few tests, can you take a look ?

[spadgett]

/approve
/lgtm

[spadgett]

/hold cancel

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[spadgett]

/retest

[spadgett]

/skip

[spadgett]

/retest

[spadgett]

/retest

[openshift-ci-robot]

@rawagner: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	6798bca27f05f2689105343132c86696de33c5e7	link	/test e2e-aws
ci/prow/e2e-aws-console-olm	f474858	link	/test e2e-aws-console-olm
ci/prow/e2e-gcp-console	f474858	link	/test e2e-gcp-console

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.