Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not remount applications between page navigations #53851

Merged
merged 2 commits into from
Dec 30, 2019
Merged

Do not remount applications between page navigations #53851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b6ce0c
Do not unmount apps when navigating within a single app
joshdover Dec 30, 2019
e14ccee
Fix Kibana logo on loading spinner
joshdover Dec 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 70 additions & 14 deletions src/core/public/application/integration_tests/router.test.tsx
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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