Skip to content

Commit

Permalink
Do not remount applications between page navigations (#53851)
Browse files Browse the repository at this point in the history
  • Loading branch information
@joshdover
joshdover authored Dec 30, 2019
1 parent b8046c7 commit 884fba2
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 45 deletions.
84 changes: 70 additions & 14 deletions src/core/public/application/integration_tests/router.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import React from 'react';
import { createMemoryHistory, History } from 'history';
import { createMemoryHistory, History, createHashHistory } from 'history';

import { AppRouter, AppNotFound } from '../ui';
import { EitherApp, MockedMounterMap, MockedMounterTuple } from '../test_types';
Expand All @@ -27,7 +27,15 @@ import { createRenderer, createAppMounter, createLegacyAppMounter } from './util
describe('AppContainer', () => {
let mounters: MockedMounterMap<EitherApp>;
let history: History;
let navigate: ReturnType<typeof createRenderer>;
let update: ReturnType<typeof createRenderer>;

const navigate = (path: string) => {
history.push(path);
return update();
};

const mockMountersToMounters = () =>
new Map([...mounters].map(([appId, { mounter }]) => [appId, mounter]));

beforeEach(() => {
mounters = new Map([
Expand All @@ -38,26 +46,26 @@ describe('AppContainer', () => {
createAppMounter('app3', '<div>App 3</div>', '/custom/path'),
] as Array<MockedMounterTuple<EitherApp>>);
history = createMemoryHistory();
navigate = createRenderer(<AppRouter history={history} mounters={mounters} />, history.push);
update = createRenderer(<AppRouter history={history} mounters={mockMountersToMounters()} />);
});

it('calls mount handler and returned unmount function when navigating between apps', async () => {
const dom1 = await navigate('/app/app1');
const app1 = mounters.get('app1')!;

expect(app1.mount).toHaveBeenCalled();
expect(app1.mounter.mount).toHaveBeenCalled();
expect(dom1?.html()).toMatchInlineSnapshot(`
"<div><div>
basename: /app/app1
html: <span>App 1</span>
</div></div>"
`);

const app1Unmount = await app1.mount.mock.results[0].value;
const app1Unmount = await app1.mounter.mount.mock.results[0].value;
const dom2 = await navigate('/app/app2');

expect(app1Unmount).toHaveBeenCalled();
expect(mounters.get('app2')!.mount).toHaveBeenCalled();
expect(mounters.get('app2')!.mounter.mount).toHaveBeenCalled();
expect(dom2?.html()).toMatchInlineSnapshot(`
"<div><div>
basename: /app/app2
Expand All @@ -70,29 +78,77 @@ describe('AppContainer', () => {
mounters.set(...createAppMounter('spaces', '<div>Custom Space</div>', '/spaces/fake-login'));
mounters.set(...createAppMounter('login', '<div>Login Page</div>', '/fake-login'));
history = createMemoryHistory();
navigate = createRenderer(<AppRouter history={history} mounters={mounters} />, history.push);
update = createRenderer(<AppRouter history={history} mounters={mockMountersToMounters()} />);

await navigate('/fake-login');

expect(mounters.get('spaces')!.mount).not.toHaveBeenCalled();
expect(mounters.get('login')!.mount).toHaveBeenCalled();
expect(mounters.get('spaces')!.mounter.mount).not.toHaveBeenCalled();
expect(mounters.get('login')!.mounter.mount).toHaveBeenCalled();
});

it('should not mount when partial route path has higher specificity', async () => {
mounters.set(...createAppMounter('login', '<div>Login Page</div>', '/fake-login'));
mounters.set(...createAppMounter('spaces', '<div>Custom Space</div>', '/spaces/fake-login'));
history = createMemoryHistory();
navigate = createRenderer(<AppRouter history={history} mounters={mounters} />, history.push);
update = createRenderer(<AppRouter history={history} mounters={mockMountersToMounters()} />);

await navigate('/spaces/fake-login');

expect(mounters.get('spaces')!.mount).toHaveBeenCalled();
expect(mounters.get('login')!.mount).not.toHaveBeenCalled();
expect(mounters.get('spaces')!.mounter.mount).toHaveBeenCalled();
expect(mounters.get('login')!.mounter.mount).not.toHaveBeenCalled();
});

it('should not remount when changing pages within app', async () => {
const { mounter, unmount } = mounters.get('app1')!;
await navigate('/app/app1/page1');
expect(mounter.mount).toHaveBeenCalledTimes(1);

// Navigating to page within app does not trigger re-render
await navigate('/app/app1/page2');
expect(mounter.mount).toHaveBeenCalledTimes(1);
expect(unmount).not.toHaveBeenCalled();
});

it('should not remount when going back within app', async () => {
const { mounter, unmount } = mounters.get('app1')!;
await navigate('/app/app1/page1');
expect(mounter.mount).toHaveBeenCalledTimes(1);

// Hitting back button within app does not trigger re-render
await navigate('/app/app1/page2');
history.goBack();
await update();
expect(mounter.mount).toHaveBeenCalledTimes(1);
expect(unmount).not.toHaveBeenCalled();
});

it('should not remount when when changing pages within app using hash history', async () => {
history = createHashHistory();
update = createRenderer(<AppRouter history={history} mounters={mockMountersToMounters()} />);

const { mounter, unmount } = mounters.get('app1')!;
await navigate('/app/app1/page1');
expect(mounter.mount).toHaveBeenCalledTimes(1);

// Changing hash history does not trigger re-render
await navigate('/app/app1/page2');
expect(mounter.mount).toHaveBeenCalledTimes(1);
expect(unmount).not.toHaveBeenCalled();
});

it('should unmount when changing between apps', async () => {
const { mounter, unmount } = mounters.get('app1')!;
await navigate('/app/app1/page1');
expect(mounter.mount).toHaveBeenCalledTimes(1);

// Navigating to other app triggers unmount
await navigate('/app/app2/page1');
expect(unmount).toHaveBeenCalledTimes(1);
});

it('calls legacy mount handler', async () => {
await navigate('/app/legacyApp1');
expect(mounters.get('legacyApp1')!.mount.mock.calls[0]).toMatchInlineSnapshot(`
expect(mounters.get('legacyApp1')!.mounter.mount.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"appBasePath": "/app/legacyApp1",
Expand All @@ -104,7 +160,7 @@ describe('AppContainer', () => {

it('handles legacy apps with subapps', async () => {
await navigate('/app/baseApp');
expect(mounters.get('baseApp:legacyApp2')!.mount.mock.calls[0]).toMatchInlineSnapshot(`
expect(mounters.get('baseApp:legacyApp2')!.mounter.mount.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"appBasePath": "/app/baseApp",
Expand Down
56 changes: 30 additions & 26 deletions src/core/public/application/integration_tests/utils.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,13 @@ import { App, LegacyApp, AppMountParameters } from '../types';
import { MockedMounter, MockedMounterTuple } from '../test_types';

type Dom = ReturnType<typeof mount> | null;
type Renderer = (item: string) => Dom | Promise<Dom>;
type Renderer = () => Dom | Promise<Dom>;

export const createRenderer = (
element: ReactElement | null,
callback?: (item: string) => void | Promise<void>
): Renderer => {
export const createRenderer = (element: ReactElement | null): Renderer => {
const dom: Dom = element && mount(<I18nProvider>{element}</I18nProvider>);

return item =>
return () =>
new Promise(async resolve => {
if (callback) {
await callback(item);
}
if (dom) {
dom.update();
}
Expand All @@ -50,29 +44,39 @@ export const createAppMounter = (
appId: string,
html: string,
appRoute = `/app/${appId}`
): MockedMounterTuple<App> => [
appId,
{
appRoute,
appBasePath: appRoute,
mount: jest.fn(async ({ appBasePath: basename, element }: AppMountParameters) => {
Object.assign(element, {
innerHTML: `<div>\nbasename: ${basename}\nhtml: ${html}\n</div>`,
});
return jest.fn(() => Object.assign(element, { innerHTML: '' }));
}),
},
];
): MockedMounterTuple<App> => {
const unmount = jest.fn();
return [
appId,
{
mounter: {
appRoute,
appBasePath: appRoute,
mount: jest.fn(async ({ appBasePath: basename, element }: AppMountParameters) => {
Object.assign(element, {
innerHTML: `<div>\nbasename: ${basename}\nhtml: ${html}\n</div>`,
});
unmount.mockImplementation(() => Object.assign(element, { innerHTML: '' }));
return unmount;
}),
},
unmount,
},
];
};

export const createLegacyAppMounter = (
appId: string,
legacyMount: MockedMounter<LegacyApp>['mount']
): MockedMounterTuple<LegacyApp> => [
appId,
{
appRoute: `/app/${appId.split(':')[0]}`,
appBasePath: `/app/${appId.split(':')[0]}`,
unmountBeforeMounting: true,
mount: legacyMount,
mounter: {
appRoute: `/app/${appId.split(':')[0]}`,
appBasePath: `/app/${appId.split(':')[0]}`,
unmountBeforeMounting: true,
mount: legacyMount,
},
unmount: jest.fn(),
},
];
14 changes: 11 additions & 3 deletions src/core/public/application/test_types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,27 @@
* under the License.
*/

import { App, LegacyApp, Mounter } from './types';
import { App, LegacyApp, Mounter, AppUnmount } from './types';
import { ApplicationService } from './application_service';

/** @internal */
export type ApplicationServiceContract = PublicMethodsOf<ApplicationService>;
/** @internal */
export type EitherApp = App | LegacyApp;
/** @internal */
export type MockedUnmount = jest.Mocked<AppUnmount>;
/** @internal */
export type MockedMounter<T extends EitherApp> = jest.Mocked<Mounter<jest.Mocked<T>>>;
/** @internal */
export type MockedMounterTuple<T extends EitherApp> = [string, MockedMounter<T>];
export type MockedMounterTuple<T extends EitherApp> = [
string,
{ mounter: MockedMounter<T>; unmount: MockedUnmount }
];
/** @internal */
export type MockedMounterMap<T extends EitherApp> = Map<string, MockedMounter<T>>;
export type MockedMounterMap<T extends EitherApp> = Map<
string,
{ mounter: MockedMounter<T>; unmount: MockedUnmount }
>;
/** @internal */
export type MockLifecycle<
T extends keyof ApplicationService,
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/application/ui/app_container.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const AppContainer: FunctionComponent<Props> = ({ mounter, appId }: Props

mount();
return unmount;
});
}, [mounter]);

return (
<Fragment>
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/rendering/views/styles.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const Styles: FunctionComponent<Props> = ({ darkMode }) => {
background-repeat: no-repeat;
background-size: contain;
/* SVG optimized according to http://codepen.io/tigt/post/optimizing-svgs-in-data-uris */
background-image: url'data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIzMCIgaGVpZ2h0PSIzOSIgdmlld0JveD0iMCAwIDMwIDM5Ij4gIDxnIGZpbGw9Im5vbmUiIGZpbGwtcnVsZT0iZXZlbm9kZCI+ICAgIDxwb2x5Z29uIGZpbGw9IiNGMDRFOTgiIHBvaW50cz0iMCAwIDAgMzQuNTQ3IDI5LjkyMiAuMDIiLz4gICAgPHBhdGggZmlsbD0iIzM0Mzc0MSIgZD0iTTAsMTQuNCBMMCwzNC41NDY4IEwxNC4yODcyLDE4LjA2MTIgQzEwLjA0MTYsMTUuNzM4IDUuMTgwNCwxNC40IDAsMTQuNCIvPiAgICA8cGF0aCBmaWxsPSIjMDBCRkIzIiBkPSJNMTcuMzc0MiwxOS45OTY4IEwyLjcyMSwzNi45MDQ4IEwxLjQzMzQsMzguMzg5MiBMMjkuMjYzOCwzOC4zODkyIEMyNy43NjE0LDMwLjgzODggMjMuNDA0MiwyNC4zMjY0IDE3LjM3NDIsMTkuOTk2OCIvPiAgPC9nPjwvc3ZnPg==');
background-image: url('data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIzMCIgaGVpZ2h0PSIzOSIgdmlld0JveD0iMCAwIDMwIDM5Ij4gIDxnIGZpbGw9Im5vbmUiIGZpbGwtcnVsZT0iZXZlbm9kZCI+ICAgIDxwb2x5Z29uIGZpbGw9IiNGMDRFOTgiIHBvaW50cz0iMCAwIDAgMzQuNTQ3IDI5LjkyMiAuMDIiLz4gICAgPHBhdGggZmlsbD0iIzM0Mzc0MSIgZD0iTTAsMTQuNCBMMCwzNC41NDY4IEwxNC4yODcyLDE4LjA2MTIgQzEwLjA0MTYsMTUuNzM4IDUuMTgwNCwxNC40IDAsMTQuNCIvPiAgICA8cGF0aCBmaWxsPSIjMDBCRkIzIiBkPSJNMTcuMzc0MiwxOS45OTY4IEwyLjcyMSwzNi45MDQ4IEwxLjQzMzQsMzguMzg5MiBMMjkuMjYzOCwzOC4zODkyIEMyNy43NjE0LDMwLjgzODggMjMuNDA0MiwyNC4zMjY0IDE3LjM3NDIsMTkuOTk2OCIvPiAgPC9nPjwvc3ZnPg==');
}
.kibanaWelcomeTitle {
Expand Down

0 comments on commit 884fba2

Please sign in to comment.