Skip to content

Commit

Permalink
memoize 'pagesFor' function to prevent infinite re-mounting
Browse files Browse the repository at this point in the history
  • Loading branch information
alecmerdler committed Jul 2, 2019
1 parent d9c3ede commit 6981ce6
Show file tree
Hide file tree
Showing 12 changed files with 149 additions and 158 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ describe('Interacting with an `AllNamespaces` install mode Operator (Redis)', ()
await element(by.linkText('Redis Enterprise Cluster')).click();
await crudView.isLoaded();

expect(crudView.statusMessageTitle.getText()).toEqual('No Application Resources Found');
expect(crudView.statusMessageDetail.getText()).toEqual('Application resources are declarative components used to define the behavior of the application.');
expect(crudView.statusMessageTitle.getText()).toEqual('No Operands Found');
expect(crudView.statusMessageDetail.getText()).toEqual('Operands are declarative components used to define the behavior of the application.');
});

it('displays YAML editor for creating a new `RedisEnterpriseCluster` instance', async() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ describe('Interacting with a `OwnNamespace` install mode Operator (Prometheus)',
expect(crudView.rowFilterFor('Alertmanager').isDisplayed()).toBe(true);
expect(crudView.rowFilterFor('ServiceMonitor').isDisplayed()).toBe(true);
expect(crudView.rowFilterFor('PrometheusRule').isDisplayed()).toBe(true);
expect(crudView.statusMessageTitle.getText()).toEqual('No Application Resources Found');
expect(crudView.statusMessageDetail.getText()).toEqual('Application resources are declarative components used to define the behavior of the application.');
expect(crudView.statusMessageTitle.getText()).toEqual('No Operands Found');
expect(crudView.statusMessageDetail.getText()).toEqual('Operands are declarative components used to define the behavior of the application.');
});

it('displays YAML editor for creating a new `Prometheus` instance', async() => {
Expand Down
7 changes: 4 additions & 3 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,15 @@
"react-dnd": "^2.6.0",
"react-dnd-html5-backend": "^2.6.0",
"react-dom": "16.8.6",
"react-helmet": "5.x",
"react-helmet": "^5.2.1",
"react-jsonschema-form": "^1.0.4",
"react-lightweight-tooltip": "1.x",
"react-linkify": "^0.2.2",
"react-measure": "^2.2.6",
"react-modal": "3.x",
"react-redux": "5.x",
"react-router-dom": "4.3.x",
"react-redux": "7.1.0",
"react-router": "5.0.1",
"react-router-dom": "5.0.1",
"react-tagsinput": "3.19.x",
"react-transition-group": "2.3.x",
"react-virtualized": "9.x",
Expand Down

This file was deleted.

8 changes: 1 addition & 7 deletions frontend/public/components/app-contents.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as _ from 'lodash-es';
import * as React from 'react';
import { connect } from 'react-redux';
import * as PropTypes from 'prop-types';
import { Redirect, Route, Switch, withRouter } from 'react-router-dom';

import { FLAGS } from '../const';
import { connectToFlags, flagPending } from '../reducers/features';
import { GlobalNotifications } from './global-notifications';
Expand All @@ -24,12 +24,6 @@ import {
PageSectionVariants,
} from '@patternfly/react-core';

// React Router's proptypes are incorrect. See https://github.com/ReactTraining/react-router/pull/5393
(Route as any).propTypes.path = PropTypes.oneOfType([
PropTypes.string,
PropTypes.arrayOf(PropTypes.string),
]);

const RedirectComponent = props => {
const to = `/k8s${props.location.pathname}`;
return <Redirect to={to} />;
Expand Down
2 changes: 1 addition & 1 deletion frontend/public/components/factory/details.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const DetailsPage = withFallback<DetailsPageProps>((props) => <Firehose r
resourceKeys={_.map(props.resources, 'prop')} />
</Firehose>, ErrorBoundaryFallback);

type Page = {
export type Page = {
href: string;
name: string;
component?: React.ComponentType<any>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
Firehose,
FirehoseResult,
StatusBox,
Page,
} from '../utils';
import { operatorGroupFor, operatorNamespaceFor } from './operator-group';
import { SubscriptionDetails } from './subscription';
Expand Down Expand Up @@ -299,19 +300,28 @@ export const CSVSubscription: React.FC<CSVSubscriptionProps> = (props) => {
</Firehose>;
};

export const ClusterServiceVersionsDetailsPage: React.StatelessComponent<ClusterServiceVersionsDetailsPageProps> = (props) => {
const AllInstances: React.SFC<{obj: ClusterServiceVersionKind}> = ({obj}) => <ProvidedAPIsPage obj={obj} />;
AllInstances.displayName = 'AllInstances';

export const ClusterServiceVersionsDetailsPage: React.FC<ClusterServiceVersionsDetailsPageProps> = (props) => {
const instancePagesFor = (obj: ClusterServiceVersionKind) => {
return (providedAPIsFor(obj).length > 1 ? [{href: 'instances', name: 'All Instances', component: AllInstances}] : [])
return (providedAPIsFor(obj).length > 1 ? [{href: 'instances', name: 'All Instances', component: ProvidedAPIsPage}] : [] as Page[])
.concat(providedAPIsFor(obj).map((desc: CRDDescription) => ({
href: referenceForProvidedAPI(desc),
name: desc.displayName,
component: (instancesProps) => <ProvidedAPIPage {...instancesProps} csv={obj} kind={referenceForProvidedAPI(desc)} namespace={props.match.params.ns} />,
component: React.memo(() => <ProvidedAPIPage csv={obj} kind={referenceForProvidedAPI(desc)} namespace={obj.metadata.namespace} />, (_.isEqual)),
})));
};

const pagesFor = React.useCallback((obj: ClusterServiceVersionKind) => _.compact([
navFactory.details(ClusterServiceVersionDetails),
navFactory.editYaml(),
{
href: 'subscription',
name: 'Subscription',
component: CSVSubscription,
},
navFactory.events(ResourceEventStream),
...instancePagesFor(obj),
]), []);

return <DetailsPage
{...props}
breadcrumbsFor={() => [
Expand All @@ -321,17 +331,7 @@ export const ClusterServiceVersionsDetailsPage: React.StatelessComponent<Cluster
namespace={props.match.params.ns}
kind={referenceForModel(ClusterServiceVersionModel)}
name={props.match.params.name}
pagesFor={(obj: ClusterServiceVersionKind) => _.compact([
navFactory.details(ClusterServiceVersionDetails),
navFactory.editYaml(),
{
href: 'subscription',
name: 'Subscription',
component: CSVSubscription,
},
navFactory.events(ResourceEventStream),
...instancePagesFor(obj),
])}
pagesFor={pagesFor}
menuActions={menuActions} />;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { connectToModel } from '../../kinds';
import { apiVersionForReference, kindForReference, K8sResourceKind, OwnerReference, K8sKind, referenceFor, GroupVersionKind, referenceForModel } from '../../module/k8s';
import { ClusterServiceVersionModel } from '../../models';
import { deleteModal } from '../modals';
import { RootState } from '../../redux';

const csvName = () => location.pathname.split('/').find((part, i, allParts) => allParts[i - 1] === referenceForModel(ClusterServiceVersionModel) || allParts[i - 1] === ClusterServiceVersionModel.plural);

Expand Down Expand Up @@ -153,7 +154,7 @@ export const OperandList: React.SFC<OperandListProps> = (props) => {
virtualize />;
};

const inFlightStateToProps = ({k8s}) => ({inFlight: k8s.getIn(['RESOURCES', 'inFlight'])});
const inFlightStateToProps = ({k8s}: RootState) => ({inFlight: k8s.getIn(['RESOURCES', 'inFlight'])});

export const ProvidedAPIsPage = connect(inFlightStateToProps)(
(props: ProvidedAPIsPageProps) => {
Expand Down Expand Up @@ -196,7 +197,8 @@ export const ProvidedAPIsPage = connect(inFlightStateToProps)(
rowFilters={firehoseResources.length > 1 ? rowFilters : null}
/>
: <StatusBox loaded={true} EmptyMsg={EmptyMsg} />;
});
}
);

export const ProvidedAPIPage = connectToModel((props: ProvidedAPIPageProps) => {
const {namespace, kind, kindsInFlight, kindObj, csv} = props;
Expand All @@ -210,9 +212,9 @@ export const ProvidedAPIPage = connectToModel((props: ProvidedAPIPageProps) => {
return <ListPage
kind={kind}
ListComponent={OperandList}
canCreate={_.get(props.kindObj, 'verbs', [] as string[]).some(v => v === 'create')}
canCreate={_.get(kindObj, 'verbs', [] as string[]).some(v => v === 'create')}
createProps={{to: `/k8s/ns/${csv.metadata.namespace}/${ClusterServiceVersionModel.plural}/${csv.metadata.name}/${kind}/~new`}}
namespace={_.get(props.kindObj, 'namespaced') ? namespace : null} />;
namespace={_.get(kindObj, 'namespaced') ? namespace : null} />;
});

export const OperandDetails = connectToModel((props: OperandDetailsProps) => {
Expand Down
79 changes: 32 additions & 47 deletions frontend/public/components/utils/horizontal-nav.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as _ from 'lodash-es';
import * as React from 'react';
import * as classNames from 'classnames';
import * as PropTypes from 'prop-types';
import { Route, Switch, Link } from 'react-router-dom';
import { History, Location } from 'history';
import { Route, Switch, Link, withRouter, match } from 'react-router-dom';

import { EmptyBox, StatusBox } from '.';
import { PodsPage } from '../pod';
Expand Down Expand Up @@ -116,7 +116,7 @@ export const navFactory: NavFactory = {
}),
};

export const NavBar: React.SFC<NavBarProps> = ({pages, basePath, hideDivider}) => {
export const NavBar = withRouter<NavBarProps>(({pages, basePath, hideDivider}) => {
// These tabs go before the divider
const before = ['', 'edit', 'yaml'];
const divider = <li className="co-m-horizontal-nav__menu-item co-m-horizontal-nav__menu-item--divider" key="_divider" />;
Expand All @@ -138,28 +138,12 @@ export const NavBar: React.SFC<NavBarProps> = ({pages, basePath, hideDivider}) =
{primaryTabs}
{secondaryTabs}
</div>;
};
});
NavBar.displayName = 'NavBar';

export class HorizontalNav extends React.PureComponent<HorizontalNavProps> {
static propTypes = {
pages: PropTypes.arrayOf(PropTypes.shape({
href: PropTypes.string,
name: PropTypes.string,
component: PropTypes.func,
})),
pagesFor: PropTypes.func,
className: PropTypes.string,
hideNav: PropTypes.bool,
hideDivider: PropTypes.bool,
match: PropTypes.shape({
path: PropTypes.string,
}),
noStatusBox: PropTypes.bool,
};

renderContent(routes: any) {
const {noStatusBox, obj, EmptyMsg, label} = this.props;
export const HorizontalNav: React.FC<HorizontalNavProps> = React.memo((props) => {
const renderContent = (routes: JSX.Element[]) => {
const {noStatusBox, obj, EmptyMsg, label} = props;
const content = <Switch> {routes} </Switch>;

const skeletonDetails = <div className="skeleton-detail-view">
Expand Down Expand Up @@ -189,31 +173,27 @@ export class HorizontalNav extends React.PureComponent<HorizontalNavProps> {
{content}
</StatusBox>
);
}
};

render() {
const props = this.props;

const componentProps = {..._.pick(props, ['filters', 'selected', 'match']), obj: _.get(props.obj, 'data')};
const extraResources = _.reduce(props.resourceKeys, (extraObjs, key) => ({...extraObjs, [key]: _.get(props[key], 'data')}), {});
const pages = props.pages || props.pagesFor(_.get(props.obj, 'data'));

const routes = pages.map(p => {
const path = `${props.match.url}/${p.href}`;
const render = () => {
return <p.component {...componentProps} {...extraResources} />;
};
return <Route path={path} exact key={p.name} render={render} />;
});

return <div className={props.className}>
<div className="co-m-horizontal-nav">
{!props.hideNav && <NavBar pages={pages} basePath={props.match.url} hideDivider={props.hideDivider} />}
{this.renderContent(routes)}
</div>
</div>;
}
}
const componentProps = {..._.pick(props, ['filters', 'selected', 'match']), obj: _.get(props.obj, 'data')};
const extraResources = _.reduce(props.resourceKeys, (extraObjs, key) => ({...extraObjs, [key]: _.get(props[key], 'data')}), {});
const pages = props.pages || props.pagesFor(_.get(props.obj, 'data'));

const routes = pages.map(p => {
const path = `${props.match.url}/${p.href}`;
const render = () => {
return <p.component {...componentProps} {...extraResources} />;
};
return <Route path={path} exact key={p.name} render={render} />;
});

return <div className={props.className}>
<div className="co-m-horizontal-nav">
{!props.hideNav && <NavBar pages={pages} basePath={props.match.url} hideDivider={props.hideDivider} />}
{renderContent(routes)}
</div>
</div>;
}, _.isEqual);

export type PodsComponentProps = {
obj: K8sResourceKind;
Expand All @@ -223,6 +203,9 @@ export type NavBarProps = {
pages: Page[];
basePath: string;
hideDivider?: boolean;
history: History;
location: Location<any>;
match: match<any>;
};

export type HorizontalNavProps = {
Expand All @@ -238,3 +221,5 @@ export type HorizontalNavProps = {
EmptyMsg?: React.ComponentType<any>;
noStatusBox?: boolean;
};

HorizontalNav.displayName = 'HorizontalNav';
16 changes: 9 additions & 7 deletions frontend/public/components/utils/router.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import * as _ from 'lodash-es';
import { createBrowserHistory, createMemoryHistory } from 'history';
import { createBrowserHistory, createMemoryHistory, History } from 'history';

type AppHistory = History & {pushPath: History['push']};

let createHistory;

Expand All @@ -15,19 +17,19 @@ try {
createHistory = createBrowserHistory;
}

export const history = createHistory({basename: window.SERVER_FLAGS.basePath});
export const history: AppHistory = createHistory({basename: window.SERVER_FLAGS.basePath});

const removeBasePath = (url = '/') => _.startsWith(url, window.SERVER_FLAGS.basePath)
? url.slice(window.SERVER_FLAGS.basePath.length - 1)
: url;

// Monkey patch history to slice off the base path
history.__replace__ = history.replace;
history.replace = url => history.__replace__(removeBasePath(url));
(history as any).__replace__ = history.replace;
history.replace = url => (history as any).__replace__(removeBasePath(url));

history.__push__ = history.push;
history.push = url => history.__push__(removeBasePath(url));
history.pushPath = path => history.__push__(path);
(history as any).__push__ = history.push;
history.push = url => (history as any).__push__(removeBasePath(url));
(history as any).pushPath = path => (history as any).__push__(path);

export const getQueryArgument = (arg: string) => new URLSearchParams(window.location.search).get(arg);

Expand Down
8 changes: 3 additions & 5 deletions frontend/public/kinds.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import * as _ from 'lodash-es';
import { connect } from 'react-redux';
import { Map as ImmutableMap } from 'immutable';
import { match } from 'react-router-dom';

import { K8sKind, K8sResourceKindReference, kindForReference, GroupVersionKind, isGroupVersionKind, allModels } from './module/k8s';
import { RootState } from './redux';

export const connectToModel = connect((state: State, props: {kind: K8sResourceKindReference} & any) => {
export const connectToModel = connect((state: RootState, props: {kind: K8sResourceKindReference} & any) => {
const kind: string = props.kind || _.get(props, 'match.params.plural');

const kindObj: K8sKind = !_.isEmpty(kind)
Expand All @@ -28,7 +28,7 @@ export type ConnectToPlural = <P extends WithPluralProps>(C: React.ComponentType
* @deprecated TODO(alecmerdler): `plural` is not a unique lookup key, remove uses of this.
* FIXME(alecmerdler): Not returning correctly typed `WrappedComponent`
*/
export const connectToPlural: ConnectToPlural = connect((state: State, props: {plural?: GroupVersionKind | string, match: match<{plural: GroupVersionKind | string}>}) => {
export const connectToPlural: ConnectToPlural = connect((state: RootState, props: {plural?: GroupVersionKind | string, match: match<{plural: GroupVersionKind | string}>}) => {
const plural = props.plural || _.get(props, 'match.params.plural');

const kindObj = isGroupVersionKind(plural)
Expand All @@ -39,5 +39,3 @@ export const connectToPlural: ConnectToPlural = connect((state: State, props: {p

return {kindObj, modelRef, kindsInFlight: state.k8s.getIn(['RESOURCES', 'inFlight'])};
});

type State = {k8s: ImmutableMap<string, any>};
Loading

0 comments on commit 6981ce6

Please sign in to comment.