From 5b87b636e7101755d4b130541b757cbe34555077 Mon Sep 17 00:00:00 2001 From: alecmerdler Date: Fri, 28 Sep 2018 16:04:57 -0400 Subject: [PATCH] prevent unnecessary re-rendering of Firehose by checking if resources are loaded when API discovery runs --- builder-run.sh | 1 + .../components/utils/firehose.spec.tsx | 66 ++++++++++++++++ .../tests/performance.scenario.ts | 10 ++- frontend/public/components/_dropdown.scss | 13 ++++ frontend/public/components/app.jsx | 1 - frontend/public/components/namespace.jsx | 21 +++--- .../public/components/source-to-image.tsx | 29 ++++--- frontend/public/components/utils/firehose.jsx | 75 +++++++++++-------- ...{horizontal-nav.jsx => horizontal-nav.tsx} | 67 ++++++++++++----- .../public/components/utils/resource-link.jsx | 2 +- frontend/tsconfig.json | 2 +- 11 files changed, 210 insertions(+), 77 deletions(-) create mode 100644 frontend/__tests__/components/utils/firehose.spec.tsx rename frontend/public/components/utils/{horizontal-nav.jsx => horizontal-nav.tsx} (71%) diff --git a/builder-run.sh b/builder-run.sh index 0fea16dfe0a..00305343bdc 100755 --- a/builder-run.sh +++ b/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 "$@" diff --git a/frontend/__tests__/components/utils/firehose.spec.tsx b/frontend/__tests__/components/utils/firehose.spec.tsx new file mode 100644 index 00000000000..1a14e69d033 --- /dev/null +++ b/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; + 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 = Firehose.WrappedComponent as any; + let wrapper: ShallowWrapper; + let resources: FirehoseResource[]; + let k8sModels: ImmutableMap; + let stopK8sWatch: Spy; + let watchK8sObject: Spy; + let watchK8sList: Spy; + + beforeEach(() => { + resources = [ + {kind: PodModel.kind, namespace: 'default', prop: 'Pod', isList: true}, + ]; + k8sModels = ImmutableMap().set('Pod', PodModel); + stopK8sWatch = jasmine.createSpy('stopK8sWatch'); + watchK8sObject = jasmine.createSpy('watchK8sObject'); + watchK8sList = jasmine.createSpy('watchK8sList'); + + wrapper = shallow(); + }); + + it('returns nothing if `props.inFlight` is true', () => { + wrapper = shallow(); + + 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); + }); +}); diff --git a/frontend/integration-tests/tests/performance.scenario.ts b/frontend/integration-tests/tests/performance.scenario.ts index 724fc2aafa1..428bb2b51c7 100644 --- a/frontend/integration-tests/tests/performance.scenario.ts +++ b/frontend/integration-tests/tests/performance.scenario.ts @@ -35,7 +35,7 @@ const chunkedRoutes = OrderedMap() 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(() => 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(() => 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(routeChunkFor, routeName); + const routeChunk = await browser.executeScript(routeChunkFor, routeName); expect(routeChunk).toBeDefined(); - expect(routeChunk.decodedBodySize).toBeLessThan(chunkLimit); + expect((routeChunk as any).decodedBodySize).toBeLessThan(chunkLimit); }); }); }); diff --git a/frontend/public/components/_dropdown.scss b/frontend/public/components/_dropdown.scss index 64fa12ffe9d..dab85a02510 100644 --- a/frontend/public/components/_dropdown.scss +++ b/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 { diff --git a/frontend/public/components/app.jsx b/frontend/public/components/app.jsx index 24a8aa735ec..59d635c9078 100644 --- a/frontend/public/components/app.jsx +++ b/frontend/public/components/app.jsx @@ -178,7 +178,6 @@ class App extends React.PureComponent { // import('./RBAC' /* webpackChunkName: "rbac" */).then(m => m.EditRulePage)} /> // import('./RBAC' /* webpackChunkName: "rbac" */).then(m => m.EditRulePage)} /> } - } /> { // import('./RBAC' /* webpackChunkName: "rbac" */).then(m => m.EditRulePage)} /> diff --git a/frontend/public/components/namespace.jsx b/frontend/public/components/namespace.jsx index 7762284573b..ebb29065dec 100644 --- a/frontend/public/components/namespace.jsx +++ b/frontend/public/components/namespace.jsx @@ -286,16 +286,19 @@ class NamespaceDropdown_ extends React.Component { const NamespaceDropdown = connect(namespaceDropdownStateToProps)(NamespaceDropdown_); -const NamespaceSelector_ = ({useProjects, inFlight}) => inFlight - ?
- : +const NamespaceSelector_ = ({useProjects, loaded}) =>
+ - ; - -const namespaceSelectorStateToProps = ({k8s}) => ({ - inFlight: k8s.getIn(['RESOURCES', 'inFlight']), - useProjects: k8s.hasIn(['RESOURCES', 'models', ProjectModel.kind]), -}); + +
; + +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_); diff --git a/frontend/public/components/source-to-image.tsx b/frontend/public/components/source-to-image.tsx index a2d53881f1f..2a56fa44320 100644 --- a/frontend/public/components/source-to-image.tsx +++ b/frontend/public/components/source-to-image.tsx @@ -89,17 +89,24 @@ class BuildSource extends React.Component { }; } - 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(); } } diff --git a/frontend/public/components/utils/firehose.jsx b/frontend/public/components/utils/firehose.jsx index 7b0aa832778..15440a1df68 100644 --- a/frontend/public/components/utils/firehose.jsx +++ b/frontend/public/components/utils/firehose.jsx @@ -116,10 +116,16 @@ const ConnectToState = connect(({k8s}, {reduxes}) => { {inject(props.children, _.omit(props, ['children', 'className', 'reduxes']))}
); -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} */ 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,32 +189,11 @@ 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, [ @@ -188,7 +201,9 @@ export const Firehose = connect( 'className', ])); - return this.props.inFlight ? null : {children} ; + return this.props.loaded || this.firehoses.length > 0 + ? {children} + : null; } } ); diff --git a/frontend/public/components/utils/horizontal-nav.jsx b/frontend/public/components/utils/horizontal-nav.tsx similarity index 71% rename from frontend/public/components/utils/horizontal-nav.jsx rename to frontend/public/components/utils/horizontal-nav.tsx index a57c69fe1c6..a24c0750185 100644 --- a/frontend/public/components/utils/horizontal-nav.jsx +++ b/frontend/public/components/utils/horizontal-nav.tsx @@ -1,3 +1,5 @@ +/* eslint-disable no-undef, no-unused-vars */ + import * as _ from 'lodash-es'; import * as React from 'react'; import * as classNames from 'classnames'; @@ -7,11 +9,12 @@ import { Route, Switch, Link } from 'react-router-dom'; import { EmptyBox, StatusBox } from '.'; import { PodsPage } from '../pod'; import { AsyncComponent } from './async'; +import { K8sResourceKind } from '../../module/k8s'; const editYamlComponent = (props) => import('../edit-yaml').then(c => c.EditYAML)} obj={props.obj} />; export const viewYamlComponent = (props) => import('../edit-yaml').then(c => c.EditYAML)} obj={props.obj} readOnly={true} />; -class PodsComponent extends React.PureComponent { +class PodsComponent extends React.PureComponent { render() { const {metadata: {namespace}, spec: {selector}} = this.props.obj; if (_.isEmpty(selector)) { @@ -25,7 +28,14 @@ class PodsComponent extends React.PureComponent { } } -export const navFactory = { +type Page = { + href: string; + name: string; + component?: React.ComponentType; +}; + +type NavFactory = {[name: string]: (c?: React.ComponentType) => Page}; +export const navFactory: NavFactory = { details: component => ({ href: '', name: 'Overview', @@ -78,8 +88,7 @@ export const navFactory = { }) }; -/** @type {React.SFC<{pages: {href: string, name: string}[], basePath: string}>} */ -export const NavBar = ({pages, basePath}) => { +export const NavBar: React.SFC = ({pages, basePath}) => { const divider =
  • ; basePath = basePath.replace(/\/$/, ''); @@ -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 { + static propTypes = { + pages: PropTypes.arrayOf(PropTypes.shape({ + href: PropTypes.string, + name: PropTypes.string, + component: PropTypes.func, + })), + className: PropTypes.string, + hideNav: PropTypes.bool, + match: PropTypes.shape({ + path: PropTypes.string, + }), + }; + render () { const props = this.props; - const componentProps = _.pick(props, ['filters', 'selected', 'match']); - componentProps.obj = props.obj.data; - const extraResources = _.reduce(props.resourceKeys, (acc, key) => ({...acc, [key]: props[key].data}), {}); + const componentProps = {..._.pick(props, ['filters', 'selected', 'match']), obj: props.obj.data}; + const extraResources = _.reduce(props.resourceKeys, (extraObjs, key) => ({...extraObjs, [key]: props[key].data}), {}); const routes = props.pages.map(p => { const path = `${props.match.url}/${p.href}`; @@ -122,15 +142,22 @@ export class HorizontalNav extends React.PureComponent { } } -HorizontalNav.propTypes = { - pages: PropTypes.arrayOf(PropTypes.shape({ - href: PropTypes.string, - name: PropTypes.string, - component: PropTypes.func, - })), - className: PropTypes.string, - hideNav: PropTypes.bool, - match: PropTypes.shape({ - path: PropTypes.string, - }), +export type PodsComponentProps = { + obj: K8sResourceKind; +}; + +export type NavBarProps = { + pages: Page[]; + basePath: string; +}; + +export type HorizontalNavProps = { + className?: string; + obj?: {loaded: boolean, data: K8sResourceKind}; + label?: string; + pages: Page[]; + match: any; + resourceKeys?: string[]; + hideNav?: boolean; + EmptyMsg?: React.ComponentType; }; diff --git a/frontend/public/components/utils/resource-link.jsx b/frontend/public/components/utils/resource-link.jsx index c4b393372c7..ef2a6957e16 100644 --- a/frontend/public/components/utils/resource-link.jsx +++ b/frontend/public/components/utils/resource-link.jsx @@ -59,7 +59,7 @@ export const resourceObjPath = (obj, kind) => resourcePath(kind, _.get(obj, 'met export const ResourceLink = connectToModel( ({className, kind, name, namespace, title, displayName, linkTo = true, kindsInFlight}) => { - if (kindsInFlight) { + if (!kind && kindsInFlight) { return null; } const path = resourcePath(kind, name, namespace); diff --git a/frontend/tsconfig.json b/frontend/tsconfig.json index 75ea14fa5e3..072cbf2ab95 100644 --- a/frontend/tsconfig.json +++ b/frontend/tsconfig.json @@ -5,7 +5,7 @@ "moduleResolution": "node", "outDir": "./public/dist/build/", "target": "es5", - "lib": ["dom", "es2015"], + "lib": ["dom", "es2015", "es2016.array.include"], "jsx": "react", "allowJs": true, "downlevelIteration": true,