Merge pull request #603 from alecmerdler/CONSOLE-812

Fix Re-Rendering Caused by API Discovery

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=512m \
        -w /go/src/github.com/openshift/console \
        $BUILDER_IMAGE "$@"

frontend/__tests__/components/utils/firehose.spec.tsx

@@ -0,0 +1,66 @@
+/* eslint-disable no-undef, no-unused-vars */
+
+import * as React from 'react';
+import { ShallowWrapper, shallow } from 'enzyme';
+import { Map as ImmutableMap } from 'immutable';
+import Spy = jasmine.Spy;
+
+import { Firehose } from '../../../public/components/utils/firehose';
+import { FirehoseResource } from '../../../public/components/factory';
+import { K8sKind, K8sResourceKindReference } from '../../../public/module/k8s';
+import { PodModel, ServiceModel } from '../../../public/models';
+
+// TODO(alecmerdler): Use these once `Firehose` is converted to TypeScript
+type FirehoseProps = {
+  expand?: boolean;
+  forceUpdate?: boolean;
+  resources: FirehoseResource[];
+
+  // Provided by `connect`
+  k8sModels: ImmutableMap<K8sResourceKindReference, K8sKind>;
+  loaded: boolean;
+  inFlight: boolean;
+  stopK8sWatch: (id: string) => void;
+  watchK8sObject: (id: string, name: string, namespace: string, query: any, k8sKind: K8sKind) => void;
+  watchK8sList: (id: string, query: any, k8sKind: K8sKind) => void;
+};
+
+describe(Firehose.displayName, () => {
+  const Component: React.ComponentType<FirehoseProps> = Firehose.WrappedComponent as any;
+  let wrapper: ShallowWrapper<FirehoseProps>;
+  let resources: FirehoseResource[];
+  let k8sModels: ImmutableMap<K8sResourceKindReference, K8sKind>;
+  let stopK8sWatch: Spy;
+  let watchK8sObject: Spy;
+  let watchK8sList: Spy;
+
+  beforeEach(() => {
+    resources = [
+      {kind: PodModel.kind, namespace: 'default', prop: 'Pod', isList: true},
+    ];
+    k8sModels = ImmutableMap<K8sResourceKindReference, K8sKind>().set('Pod', PodModel);
+    stopK8sWatch = jasmine.createSpy('stopK8sWatch');
+    watchK8sObject = jasmine.createSpy('watchK8sObject');
+    watchK8sList = jasmine.createSpy('watchK8sList');
+
+    wrapper = shallow(<Component resources={resources} k8sModels={k8sModels} loaded={true} inFlight={false} stopK8sWatch={stopK8sWatch} watchK8sObject={watchK8sObject} watchK8sList={watchK8sList} />);
+  });
+
+  it('returns nothing if `props.inFlight` is true', () => {
+    wrapper = shallow(<Component resources={resources} k8sModels={k8sModels} loaded={false} inFlight={true} stopK8sWatch={stopK8sWatch} watchK8sObject={watchK8sObject} watchK8sList={watchK8sList} />);
+
+    expect(wrapper.html()).toBeNull();
+  });
+
+  it('does not re-render when `props.inFlight` changes but Firehose data is loaded', () => {
+    expect(wrapper.instance().shouldComponentUpdate({inFlight: true, loaded: true} as FirehoseProps, null, null)).toBe(false);
+  });
+
+  it('clears and restarts "firehoses" when `props.resources` change', () => {
+    resources = resources.concat([{kind: ServiceModel.kind, namespace: 'default', prop: 'Service', isList: true}]);
+    wrapper = wrapper.setProps({resources});
+
+    expect(stopK8sWatch.calls.count()).toEqual(1);
+    expect(watchK8sList.calls.count()).toEqual(2);
+  });
+});

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

@@ -35,7 +35,7 @@ const chunkedRoutes = OrderedMap<string, {section: string, name: string}>()
 describe('Performance test', () => {
 
   it('checks bundle size using ResourceTiming API', async() => {
-    const resources = await browser.executeScript<{name: string, size: number}[]>(() => performance.getEntriesByType('resource')
+    const resources = await browser.executeScript<string[]>(() => performance.getEntriesByType('resource')
       .filter(({name}) => name.endsWith('.js') && name.indexOf('main') > -1 && name.indexOf('runtime') === -1)
       .map(({name, decodedBodySize}) => ({name: name.split('/').slice(-1)[0], size: Math.floor(decodedBodySize / 1024)}))
       .reduce((acc, val) => acc.concat(`${val.name.split('-')[0]}: ${val.size} KB, `), '')
@@ -47,7 +47,7 @@ describe('Performance test', () => {
   });
 
   it('downloads new bundle for "Overview" route', async() => {
-    browser.get(`${appHost}/status/all-namespaces`);
+    await browser.get(`${appHost}/status/all-namespaces`);
     await browser.wait(until.presenceOf(crudView.resourceTitle));
 
     const overviewChunk = await browser.executeScript<any>(() => performance.getEntriesByType('resource')
@@ -82,13 +82,15 @@ describe('Performance test', () => {
     };
 
     it(`downloads new bundle for ${routeName}`, async() => {
+      await browser.get(`${appHost}/status/all-namespaces`);
+      await browser.wait(until.presenceOf(crudView.resourceTitle));
       await sidenavView.clickNavLink([route.section, route.name]);
       await crudView.isLoaded();
 
-      const routeChunk = await browser.executeScript<any>(routeChunkFor, routeName);
+      const routeChunk = await browser.executeScript<PerformanceEntry>(routeChunkFor, routeName);
 
       expect(routeChunk).toBeDefined();
-      expect(routeChunk.decodedBodySize).toBeLessThan(chunkLimit);
+      expect((routeChunk as any).decodedBodySize).toBeLessThan(chunkLimit);
     });
   });
 });

frontend/public/components/_dropdown.scss

@@ -263,6 +263,19 @@ $color-bookmarker: #DDD;
   }
 }
 
+.co-ns-selector {
+  height: 42px;
+  transition: 0.4s;
+}
+
+.co-ns-selector--hidden {
+  opacity: 0;
+}
+
+.co-ns-selector--visible {
+  opacity: 1;
+}
+
 .btn-dropdown {
   max-width: 100%;
   .caret {

frontend/public/components/app.jsx

@@ -178,7 +178,6 @@ class App extends React.PureComponent {
               // <LazyRoute path="/k8s/cluster/clusterroles/:name/add-rule" exact loader={() => import('./RBAC' /* webpackChunkName: "rbac" */).then(m => m.EditRulePage)} />
               // <LazyRoute path="/k8s/cluster/clusterroles/:name/:rule/edit" exact loader={() => import('./RBAC' /* webpackChunkName: "rbac" */).then(m => m.EditRulePage)} />
             }
-            <Route path="/k8s/cluster/clusterroles/:name" component={props => <ResourceDetailsPage {...props} plural="clusterroles" />} />
 
             {
               // <LazyRoute path="/k8s/ns/:ns/roles/:name/add-rule" exact loader={() => import('./RBAC' /* webpackChunkName: "rbac" */).then(m => m.EditRulePage)} />

frontend/public/components/namespace.jsx

@@ -286,16 +286,19 @@ class NamespaceDropdown_ extends React.Component {
 
 const NamespaceDropdown = connect(namespaceDropdownStateToProps)(NamespaceDropdown_);
 
-const NamespaceSelector_ = ({useProjects, inFlight}) => inFlight
-  ? <div className="co-namespace-selector" />
-  : <Firehose resources={[{ kind: getModel(useProjects).kind, prop: 'namespace', isList: true }]}>
+const NamespaceSelector_ = ({useProjects, loaded}) => <div className={`co-ns-selector co-ns-selector--${!loaded ? 'hidden' : 'visible'}`}>
+  <Firehose resources={[{kind: getModel(useProjects).kind, prop: 'namespace', isList: true}]}>
     <NamespaceDropdown useProjects={useProjects} />
-  </Firehose>;
-
-const namespaceSelectorStateToProps = ({k8s}) => ({
-  inFlight: k8s.getIn(['RESOURCES', 'inFlight']),
-  useProjects: k8s.hasIn(['RESOURCES', 'models', ProjectModel.kind]),
-});
+  </Firehose>
+</div>;
+
+const namespaceSelectorStateToProps = ({k8s}) => {
+  const useProjects = k8s.hasIn(['RESOURCES', 'models', ProjectModel.kind]);
+  return {
+    useProjects,
+    loaded: k8s.getIn([useProjects ? 'projects' : 'namespaces', 'loaded']),
+  };
+};
 
 export const NamespaceSelector = connect(namespaceSelectorStateToProps)(NamespaceSelector_);
 

frontend/public/components/source-to-image.tsx

@@ -89,17 +89,24 @@ class BuildSource extends React.Component<BuildSourceProps, BuildSourceState> {
     };
   }
 
-  componentDidUpdate(prevProps) {
-    const { obj } = this.props;
-    if (obj !== prevProps.obj && obj.loaded) {
-      const previousTag = this.state.selectedTag;
-      // Sort tags in reverse order by semver, falling back to a string comparison if not a valid version.
-      const tags = getBuilderTagsSortedByVersion(obj.data);
-      // Select the first tag if the current tag is missing or empty.
-      const selectedTag = previousTag && _.includes(tags, previousTag)
-        ? previousTag
-        : _.get(_.head(tags), 'name');
-      this.setState({tags, selectedTag}, this.getImageStreamImage);
+  static getDerivedStateFromProps(props, state) {
+    if (_.isEmpty(props.obj.data)) {
+      return null;
+    }
+    const previousTag = state.selectedTag;
+    // Sort tags in reverse order by semver, falling back to a string comparison if not a valid version.
+    const tags = getBuilderTagsSortedByVersion(props.obj.data);
+    // Select the first tag if the current tag is missing or empty.
+    const selectedTag = previousTag && _.includes(tags, previousTag)
+      ? previousTag
+      : _.get(_.head(tags), 'name');
+
+    return {tags, selectedTag};
+  }
+
+  componentDidUpdate(prevProps, prevState) {
+    if (prevState.selectedTag !== this.state.selectedTag) {
+      this.getImageStreamImage();
     }
   }
 

frontend/public/components/utils/firehose.jsx

@@ -116,10 +116,16 @@ const ConnectToState = connect(({k8s}, {reduxes}) => {
   {inject(props.children, _.omit(props, ['children', 'className', 'reduxes']))}
 </div>);
 
-const stateToProps = ({k8s}, {resources}) => ({
-  k8sModels: resources.reduce((models, {kind}) => models.set(kind, k8s.getIn(['RESOURCES', 'models', kind])), ImmutableMap()),
-  inFlight: k8s.getIn(['RESOURCES', 'inFlight']),
-});
+const stateToProps = ({k8s}, {resources}) => {
+  const k8sModels = resources.reduce((models, {kind}) => models.set(kind, k8s.getIn(['RESOURCES', 'models', kind])), ImmutableMap());
+  const loaded = (r) => k8s.getIn([makeReduxID(k8sModels.get(r.kind), makeQuery(r.namespace, r.selector, r.fieldSelector, r.name)), 'loaded']);
+
+  return {
+    k8sModels,
+    loaded: resources.every(loaded),
+    inFlight: k8s.getIn(['RESOURCES', 'inFlight']),
+  };
+};
 
 export const Firehose = connect(
   stateToProps, {
@@ -129,8 +135,36 @@ export const Firehose = connect(
   })(
   /** @augments {React.Component<{k8sModels?: Map<string, K8sKind}>} */
   class Firehose extends React.Component {
-    componentWillMount (props=this.props) {
-      const { watchK8sList, watchK8sObject, resources, k8sModels, inFlight } = props;
+    // TODO: Convert this to `componentDidMount`
+    // eslint-disable-next-line camelcase
+    UNSAFE_componentWillMount () {
+      this.start();
+    }
+
+    componentWillUnmount () {
+      this.clear();
+    }
+
+    shouldComponentUpdate(nextProps) {
+      const {forceUpdate = false} = this.props;
+      if (nextProps.inFlight !== this.props.inFlight && nextProps.loaded) {
+        return forceUpdate;
+      }
+      return true;
+    }
+
+    componentDidUpdate(prevProps) {
+      const discoveryComplete = !this.props.inFlight && !this.props.loaded && this.firehoses.length === 0;
+      const resourcesChanged = _.intersectionWith(prevProps.resources, this.props.resources, _.isEqual).length !== this.props.resources.length;
+
+      if (discoveryComplete || resourcesChanged) {
+        this.clear();
+        this.start();
+      }
+    }
+
+    start() {
+      const { watchK8sList, watchK8sObject, resources, k8sModels, inFlight } = this.props;
 
       if (inFlight) {
         this.firehoses = [];
@@ -155,40 +189,21 @@ export const Firehose = connect(
       );
     }
 
-    componentWillUnmount () {
-      const { stopK8sWatch } = this.props;
-
-      this.firehoses.forEach(({id}) => stopK8sWatch(id));
+    clear() {
+      this.firehoses.forEach(({id}) => this.props.stopK8sWatch(id));
       this.firehoses = [];
     }
 
-    shouldComponentUpdate(nextProps, nextState, nextContext) {
-      const {resources: currentResources, forceUpdate = false} = this.props;
-
-      const { resources, expand, inFlight } = nextProps;
-
-      if (_.intersectionWith(resources, currentResources, _.isEqual).length === resources.length && inFlight === this.props.inFlight) {
-        if (_.get(nextContext, 'router.route.location.pathname') !== _.get(this.context, 'router.route.location.pathname')) {
-          return true;
-        }
-        if (expand !== this.props.expand) {
-          return true;
-        }
-        return forceUpdate;
-      }
-      this.componentWillUnmount();
-      this.componentWillMount(nextProps);
-      return true;
-    }
-
     render () {
       const reduxes = this.firehoses.map(({id, prop, isList, filters, optional}) => ({reduxID: id, prop, isList, filters, optional}));
       const children = inject(this.props.children, _.omit(this.props, [
         'children',
         'className',
       ]));
 
-      return this.props.inFlight ? null : <ConnectToState reduxes={reduxes}> {children} </ConnectToState>;
+      return this.props.loaded || this.firehoses.length > 0
+        ? <ConnectToState reduxes={reduxes}> {children} </ConnectToState>
+        : null;
     }
   }
 );