Fix Re-Rendering Caused by API Discovery #603

alecmerdler · 2018-09-28T20:08:48Z

Description

Refactor Firehose component to simplify complex/hacky React lifecyle logic. Ensure that Firehose does not re-render when API discovery runs if its own data is loaded.

Testing

Open a terminal alongside the browser window and create a CRD:

$ kubectl create -f https://gist.githubusercontent.com/alecmerdler/df6a32ef511721fd8e633251a8dbc140/raw/d272d015756c96651fc8a0cfbce5cdeab902d0d5/cluster.crd.yaml

The list/detail view should not be affected.

Addresses https://jira.coreos.com/browse/CONSOLE-812

spadgett · 2018-09-28T20:45:38Z

This seems to make things much better, but I'm still seeing some flicker.

I wish we better understood why the websocket keeps closing:

WS closed abnormally - starting polling loop over! k8s-actions.ts:216
destroying websocket: /api/kubernetes/apis/apiregistration.k8s.io/v1/apiservices?watch=true&resourceVersion=106518 ws-factory.js:250
apiservices timed out - restarting polling

alecmerdler · 2018-09-28T20:48:01Z

@spadgett When writing the PackageManifest API server, I was seeing this in the logs occacionally:

streamwatcher.go:103] Unexpected EOF during watch stream event decoding: unexpected EOF

This may be a problem with Kubernetes API server code in general. Regardless, this fix will work for non-admin users who have to use polling instead of listing apiservices.

spadgett · 2018-09-28T20:57:33Z

I'm seeing some other errors with this change:

No model registered for Pod firehose.jsx:146
[Show/hide message details.] TypeError: props.obj is undefined[Learn More] horizontal-nav.jsx:103
./public/components/utils/horizontal-nav.jsx/HorizontalNav.prototype.render
horizontal-nav.jsx:103

alecmerdler · 2018-10-01T20:25:15Z

frontend/public/components/namespace.jsx

@@ -281,16 +281,20 @@ class NamespaceDropdown_ extends React.Component {

 const NamespaceDropdown = connect(namespaceDropdownStateToProps)(NamespaceDropdown_);

-const NamespaceSelector_ = ({useProjects, inFlight}) => inFlight
+const NamespaceSelector_ = ({useProjects, inFlight, loaded}) => inFlight && !loaded


Need to prevent this component from re-rendering.

alecmerdler · 2018-10-01T20:25:48Z

frontend/public/components/utils/horizontal-nav.tsx

@@ -94,14 +103,25 @@ export const NavBar = ({pages, basePath}) => {
 };
 NavBar.displayName = 'NavBar';

-/** @augments {React.PureComponent<{className?: string, label?: string, pages: {href: string, name: string, component: React.ComponentType}[], match: any, resourceKeys?: string[]}>} */
-export class HorizontalNav extends React.PureComponent {
+export class HorizontalNav extends React.PureComponent<HorizontalNavProps> {


No changes here other than converting to TypeScript.

alecmerdler · 2018-10-01T20:28:20Z

frontend/public/components/utils/firehose.jsx

  k8sModels: resources.reduce((models, {kind}) => models.set(kind, k8s.getIn(['RESOURCES', 'models', kind])), ImmutableMap()),
+  loaded: reduxIDs.every(id => k8s.getIn([id, 'loaded'])),


@spadgett So this solution prevents the entire view from being re-rendered, but something is causing Firehose to be unmounted when RESOURCES.inFlight changes, which causes the WebSockets to be briefly terminated. However, the re-rendering only affects the list rows now and is pretty fast.

spadgett

Thanks @alecmerdler. This is a huge improvement 👍

We can investigate Firehose getting unmounted separately.

/lgtm

spadgett · 2018-10-02T16:18:11Z

/hold

spadgett · 2018-10-02T16:18:42Z

Holding to make sure our CI runs

alecmerdler · 2018-10-05T22:35:38Z

builder-run.sh

@@ -30,5 +30,6 @@ docker run $ENV_STR --rm --net=host \
       --user="${BUILDER_RUN_USER}" \
       $VOLUME_MOUNT \
       -v "$(pwd)":/go/src/github.com/openshift/console \
+       --shm-size=256m \


Used this suggestion to fix the WebDriver error.

… are loaded when API discovery runs

alecmerdler · 2018-10-09T20:00:26Z

frontend/integration-tests/tests/performance.scenario.ts

@@ -82,13 +82,15 @@ describe('Performance test', () => {
    };

    it(`downloads new bundle for ${routeName}`, async() => {
+      await browser.get(`${appHost}/status/all-namespaces`);


Adding this fixes the weird "Performance" e2e test failure.

spadgett · 2018-10-09T22:13:17Z

👍

/lgtm

alecmerdler requested a review from spadgett September 28, 2018 20:08

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 28, 2018

alecmerdler force-pushed the CONSOLE-812 branch 3 times, most recently from 2df679f to 993bd2d Compare October 1, 2018 14:46

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2018

alecmerdler force-pushed the CONSOLE-812 branch 7 times, most recently from a5702bc to 2664136 Compare October 1, 2018 20:20

alecmerdler commented Oct 1, 2018

View reviewed changes

alecmerdler force-pushed the CONSOLE-812 branch from 2664136 to 341d26f Compare October 2, 2018 14:10

openshift-ci-robot assigned spadgett Oct 2, 2018

spadgett approved these changes Oct 2, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2018

alecmerdler force-pushed the CONSOLE-812 branch from 341d26f to a4029fe Compare October 2, 2018 20:51

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2018

alecmerdler force-pushed the CONSOLE-812 branch from e8cf603 to 18578c9 Compare October 5, 2018 21:47

alecmerdler commented Oct 5, 2018

View reviewed changes

alecmerdler force-pushed the CONSOLE-812 branch 7 times, most recently from 74093b7 to 59c5187 Compare October 8, 2018 15:24

openshift deleted a comment from spadgett Oct 8, 2018

alecmerdler force-pushed the CONSOLE-812 branch 11 times, most recently from c8e0fa3 to cb24649 Compare October 9, 2018 17:49

prevent unnecessary re-rendering of Firehose by checking if resources…

5b87b63

… are loaded when API discovery runs

alecmerdler force-pushed the CONSOLE-812 branch from cb24649 to 5b87b63 Compare October 9, 2018 18:58

alecmerdler commented Oct 9, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2018

openshift-merge-robot merged commit 58cdcbb into openshift:master Oct 9, 2018

alecmerdler deleted the CONSOLE-812 branch October 9, 2018 22:41

rhamilto mentioned this pull request Oct 10, 2018

Remove height on .co-ns-selector to fix bg color bug on catalog at mo… #638

Closed

spadgett mentioned this pull request Oct 24, 2018

Cache API discovery #695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Re-Rendering Caused by API Discovery #603

Fix Re-Rendering Caused by API Discovery #603

alecmerdler commented Sep 28, 2018 •

edited

Loading

spadgett commented Sep 28, 2018

alecmerdler commented Sep 28, 2018 •

edited

Loading

spadgett commented Sep 28, 2018

alecmerdler Oct 1, 2018

alecmerdler Oct 1, 2018

alecmerdler Oct 1, 2018

spadgett left a comment

spadgett commented Oct 2, 2018

spadgett commented Oct 2, 2018

alecmerdler Oct 5, 2018 •

edited

Loading

alecmerdler Oct 9, 2018

spadgett commented Oct 9, 2018

		k8sModels: resources.reduce((models, {kind}) => models.set(kind, k8s.getIn(['RESOURCES', 'models', kind])), ImmutableMap()),
		loaded: reduxIDs.every(id => k8s.getIn([id, 'loaded'])),

Fix Re-Rendering Caused by API Discovery #603

Fix Re-Rendering Caused by API Discovery #603

Conversation

alecmerdler commented Sep 28, 2018 • edited Loading

Description

Testing

spadgett commented Sep 28, 2018

alecmerdler commented Sep 28, 2018 • edited Loading

spadgett commented Sep 28, 2018

alecmerdler Oct 1, 2018

Choose a reason for hiding this comment

alecmerdler Oct 1, 2018

Choose a reason for hiding this comment

alecmerdler Oct 1, 2018

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Oct 2, 2018

spadgett commented Oct 2, 2018

alecmerdler Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

alecmerdler Oct 9, 2018

Choose a reason for hiding this comment

spadgett commented Oct 9, 2018

alecmerdler commented Sep 28, 2018 •

edited

Loading

alecmerdler commented Sep 28, 2018 •

edited

Loading

alecmerdler Oct 5, 2018 •

edited

Loading