From e873848cc10e7c900946b87d15086d32959029a4 Mon Sep 17 00:00:00 2001 From: Constance Date: Tue, 29 Sep 2020 08:18:58 -0700 Subject: [PATCH] [Enterprise Search] DRY out createHref helper for navigateToUrl calls (#78717) * DRY out createHref helper - that navigateToUrl will shortly also use + fix mockHistory type complaint * KibanaLogic: Update navigateToUrl to use createHref helper + add param.history value, since we're technically supposed to be using Kibana's passed history over anything from useHistory * Update breadcrumbs & eui link helpers w/ new KibanaLogic changes - navigateToUrl is now double-dipping createHref, so we update it to not do so - remove useHistory in favor of grabbing history from KibanaLogic * [Optional?] Update FlashMessages to grab history from KibanaLogic - since we're now storing it there, we might as well grab it as well instead of passing in props? * [Misc] Fix failing tests due to react router mock --- .../__mocks__/kibana_logic.mock.ts | 3 +++ .../__mocks__/react_router_history.mock.ts | 3 ++- .../public/applications/index.tsx | 5 ++-- .../flash_messages_logic.test.ts | 5 +++- .../flash_messages/flash_messages_logic.ts | 13 ++++----- .../set_message_helpers.test.ts | 5 +++- .../shared/kibana/kibana_logic.test.ts | 21 ++++++++++++++- .../shared/kibana/kibana_logic.ts | 20 +++++++++++--- .../generate_breadcrumbs.test.ts | 16 ++++++----- .../kibana_chrome/generate_breadcrumbs.ts | 12 +++------ .../react_router_helpers/create_href.test.ts | 19 +++++++++++++ .../react_router_helpers/create_href.ts | 27 +++++++++++++++++++ .../react_router_helpers/eui_link.test.tsx | 1 - .../shared/react_router_helpers/eui_link.tsx | 10 +++---- .../shared/react_router_helpers/index.ts | 1 + 15 files changed, 122 insertions(+), 39 deletions(-) create mode 100644 x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.test.ts create mode 100644 x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.ts diff --git a/x-pack/plugins/enterprise_search/public/applications/__mocks__/kibana_logic.mock.ts b/x-pack/plugins/enterprise_search/public/applications/__mocks__/kibana_logic.mock.ts index 9f3c2443bc9b8..419db86148984 100644 --- a/x-pack/plugins/enterprise_search/public/applications/__mocks__/kibana_logic.mock.ts +++ b/x-pack/plugins/enterprise_search/public/applications/__mocks__/kibana_logic.mock.ts @@ -4,8 +4,11 @@ * you may not use this file except in compliance with the Elastic License. */ +import { mockHistory } from './'; + export const mockKibanaValues = { config: { host: 'http://localhost:3002' }, + history: mockHistory, navigateToUrl: jest.fn(), setBreadcrumbs: jest.fn(), setDocTitle: jest.fn(), diff --git a/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts b/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts index 7b3ac86ad0ab1..2c833bcfeaf4c 100644 --- a/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts +++ b/x-pack/plugins/enterprise_search/public/applications/__mocks__/react_router_history.mock.ts @@ -15,7 +15,7 @@ export const mockHistory = { pathname: '/current-path', }, listen: jest.fn(() => jest.fn()), -}; +} as any; export const mockLocation = { key: 'someKey', pathname: '/current-path', @@ -25,6 +25,7 @@ export const mockLocation = { }; jest.mock('react-router-dom', () => ({ + ...(jest.requireActual('react-router-dom') as object), useHistory: jest.fn(() => mockHistory), useLocation: jest.fn(() => mockLocation), })); diff --git a/x-pack/plugins/enterprise_search/public/applications/index.tsx b/x-pack/plugins/enterprise_search/public/applications/index.tsx index 63be9b684e56f..8b5e7daea989c 100644 --- a/x-pack/plugins/enterprise_search/public/applications/index.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/index.tsx @@ -41,6 +41,7 @@ export const renderApp = ( const unmountKibanaLogic = mountKibanaLogic({ config, + history: params.history, navigateToUrl: core.application.navigateToUrl, setBreadcrumbs: core.chrome.setBreadcrumbs, setDocTitle: core.chrome.docTitle.change, @@ -53,9 +54,7 @@ export const renderApp = ( errorConnecting, readOnlyMode: initialData.readOnlyMode, }); - const unmountFlashMessagesLogic = mountFlashMessagesLogic({ - history: params.history, - }); + const unmountFlashMessagesLogic = mountFlashMessagesLogic(); ReactDOM.render( diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.test.ts index c12011b47a472..ff1ec7428e828 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.test.ts @@ -7,11 +7,14 @@ import { resetContext } from 'kea'; import { mockHistory } from '../../__mocks__'; +jest.mock('../kibana', () => ({ + KibanaLogic: { values: { history: mockHistory } }, +})); import { FlashMessagesLogic, mountFlashMessagesLogic, IFlashMessage } from './'; describe('FlashMessagesLogic', () => { - const mount = () => mountFlashMessagesLogic({ history: mockHistory as any }); + const mount = () => mountFlashMessagesLogic(); beforeEach(() => { jest.clearAllMocks(); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.ts index 1735cc8ac7228..5a05a03adeb6b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/flash_messages_logic.ts @@ -6,7 +6,8 @@ import { kea, MakeLogicType } from 'kea'; import { ReactNode } from 'react'; -import { History } from 'history'; + +import { KibanaLogic } from '../kibana'; export interface IFlashMessage { type: 'success' | 'info' | 'warning' | 'error'; @@ -61,10 +62,10 @@ export const FlashMessagesLogic = kea ({ + events: ({ values, actions }) => ({ afterMount: () => { // On React Router navigation, clear previous flash messages and load any queued messages - const unlisten = props.history.listen(() => { + const unlisten = KibanaLogic.values.history.listen(() => { actions.clearFlashMessages(); actions.setFlashMessages(values.queuedMessages); actions.clearQueuedMessages(); @@ -81,11 +82,7 @@ export const FlashMessagesLogic = kea { - FlashMessagesLogic(props); +export const mountFlashMessagesLogic = () => { const unmount = FlashMessagesLogic.mount(); return unmount; }; diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/set_message_helpers.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/set_message_helpers.test.ts index f2ddd560ac9c1..46027fdfb22b1 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/set_message_helpers.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/set_message_helpers.test.ts @@ -5,6 +5,9 @@ */ import { mockHistory } from '../../__mocks__'; +jest.mock('../kibana', () => ({ + KibanaLogic: { values: { history: mockHistory } }, +})); import { FlashMessagesLogic, @@ -18,7 +21,7 @@ describe('Flash Message Helpers', () => { const message = 'I am a message'; beforeEach(() => { - mountFlashMessagesLogic({ history: mockHistory as any }); + mountFlashMessagesLogic(); }); it('setSuccessMessage()', () => { diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts index 99262cca97ca8..4d51362a7e11b 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts @@ -20,7 +20,10 @@ describe('KibanaLogic', () => { it('sets values from props', () => { mountKibanaLogic(mockKibanaValues); - expect(KibanaLogic.values).toEqual(mockKibanaValues); + expect(KibanaLogic.values).toEqual({ + ...mockKibanaValues, + navigateToUrl: expect.any(Function), + }); }); it('gracefully handles missing configs', () => { @@ -29,4 +32,20 @@ describe('KibanaLogic', () => { expect(KibanaLogic.values.config).toEqual({}); }); }); + + describe('navigateToUrl()', () => { + beforeEach(() => mountKibanaLogic(mockKibanaValues)); + + it('runs paths through createHref before calling navigateToUrl', () => { + KibanaLogic.values.navigateToUrl('/test'); + + expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/app/enterprise_search/test'); + }); + + it('does not run paths through createHref if the shouldNotCreateHref option is passed', () => { + KibanaLogic.values.navigateToUrl('/test', { shouldNotCreateHref: true }); + + expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/test'); + }); + }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.ts b/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.ts index a884acb02d10a..5904c6c89e39c 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.ts @@ -6,26 +6,40 @@ import { kea, MakeLogicType } from 'kea'; +import { History } from 'history'; import { ApplicationStart, ChromeBreadcrumb } from 'src/core/public'; -export interface IKibanaValues { +import { createHref, ICreateHrefOptions } from '../react_router_helpers'; + +interface IKibanaLogicProps { config: { host?: string }; + history: History; navigateToUrl: ApplicationStart['navigateToUrl']; setBreadcrumbs(crumbs: ChromeBreadcrumb[]): void; setDocTitle(title: string): void; } +export interface IKibanaValues extends IKibanaLogicProps { + navigateToUrl(path: string, options?: ICreateHrefOptions): Promise; +} export const KibanaLogic = kea>({ path: ['enterprise_search', 'kibana_logic'], reducers: ({ props }) => ({ config: [props.config || {}, {}], - navigateToUrl: [props.navigateToUrl, {}], + history: [props.history, {}], + navigateToUrl: [ + (url: string, options?: ICreateHrefOptions) => { + const href = createHref(url, props.history, options); + return props.navigateToUrl(href); + }, + {}, + ], setBreadcrumbs: [props.setBreadcrumbs, {}], setDocTitle: [props.setDocTitle, {}], }), }); -export const mountKibanaLogic = (props: IKibanaValues) => { +export const mountKibanaLogic = (props: IKibanaLogicProps) => { KibanaLogic(props); const unmount = KibanaLogic.mount(); return unmount; diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts index a2c0bcae6fc18..61a4397486346 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.test.ts @@ -5,10 +5,12 @@ */ import '../../__mocks__/kea.mock'; -import '../../__mocks__/react_router_history.mock'; import { mockKibanaValues, mockHistory } from '../../__mocks__'; -jest.mock('../react_router_helpers', () => ({ letBrowserHandleEvent: jest.fn(() => false) })); +jest.mock('../react_router_helpers', () => ({ + letBrowserHandleEvent: jest.fn(() => false), + createHref: jest.requireActual('../react_router_helpers').createHref, +})); import { letBrowserHandleEvent } from '../react_router_helpers'; import { @@ -50,21 +52,23 @@ describe('useBreadcrumbs', () => { it('prevents default navigation and uses React Router history on click', () => { const breadcrumb = useBreadcrumbs([{ text: '', path: '/test' }])[0] as any; + + expect(breadcrumb.href).toEqual('/app/enterprise_search/test'); + expect(mockHistory.createHref).toHaveBeenCalled(); + const event = { preventDefault: jest.fn() }; breadcrumb.onClick(event); - expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/app/enterprise_search/test'); - expect(mockHistory.createHref).toHaveBeenCalled(); expect(event.preventDefault).toHaveBeenCalled(); + expect(mockKibanaValues.navigateToUrl).toHaveBeenCalled(); }); it('does not call createHref if shouldNotCreateHref is passed', () => { const breadcrumb = useBreadcrumbs([ { text: '', path: '/test', shouldNotCreateHref: true }, ])[0] as any; - breadcrumb.onClick({ preventDefault: () => null }); - expect(mockKibanaValues.navigateToUrl).toHaveBeenCalledWith('/test'); + expect(breadcrumb.href).toEqual('/test'); expect(mockHistory.createHref).not.toHaveBeenCalled(); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts index ff7f29e2e393c..9ef23e6b176d9 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/kibana_chrome/generate_breadcrumbs.ts @@ -5,7 +5,6 @@ */ import { useValues } from 'kea'; -import { useHistory } from 'react-router-dom'; import { EuiBreadcrumb } from '@elastic/eui'; import { KibanaLogic } from '../../shared/kibana'; @@ -16,7 +15,7 @@ import { WORKPLACE_SEARCH_PLUGIN, } from '../../../../common/constants'; -import { letBrowserHandleEvent } from '../react_router_helpers'; +import { letBrowserHandleEvent, createHref } from '../react_router_helpers'; /** * Generate React-Router-friendly EUI breadcrumb objects @@ -33,20 +32,17 @@ interface IBreadcrumb { export type TBreadcrumbs = IBreadcrumb[]; export const useBreadcrumbs = (breadcrumbs: TBreadcrumbs) => { - const history = useHistory(); - const { navigateToUrl } = useValues(KibanaLogic); + const { navigateToUrl, history } = useValues(KibanaLogic); return breadcrumbs.map(({ text, path, shouldNotCreateHref }) => { const breadcrumb = { text } as EuiBreadcrumb; if (path) { - const href = shouldNotCreateHref ? path : (history.createHref({ pathname: path }) as string); - - breadcrumb.href = href; + breadcrumb.href = createHref(path, history, { shouldNotCreateHref }); breadcrumb.onClick = (event) => { if (letBrowserHandleEvent(event)) return; event.preventDefault(); - navigateToUrl(href); + navigateToUrl(path, { shouldNotCreateHref }); }; } diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.test.ts b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.test.ts new file mode 100644 index 0000000000000..5f96beeb42ae4 --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.test.ts @@ -0,0 +1,19 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { mockHistory } from '../../__mocks__'; + +import { createHref } from './'; + +describe('createHref', () => { + it('generates a path with the React Router basename included', () => { + expect(createHref('/test', mockHistory)).toEqual('/app/enterprise_search/test'); + }); + + it('does not include the basename if shouldNotCreateHref is passed', () => { + expect(createHref('/test', mockHistory, { shouldNotCreateHref: true })).toEqual('/test'); + }); +}); diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.ts b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.ts new file mode 100644 index 0000000000000..cc8279c80a092 --- /dev/null +++ b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/create_href.ts @@ -0,0 +1,27 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { History } from 'history'; + +/** + * This helper uses React Router's createHref function to generate links with router basenames accounted for. + * For example, if we perform navigateToUrl('/engines') within App Search, we expect the app basename + * to be taken into account to be intelligently routed to '/app/enterprise_search/app_search/engines'. + * + * This helper accomplishes that, while still giving us an escape hatch for navigation *between* apps. + * For example, if we want to navigate the user from App Search to Enterprise Search we could + * navigateToUrl('/app/enterprise_search', { shouldNotCreateHref: true }) + */ +export interface ICreateHrefOptions { + shouldNotCreateHref?: boolean; +} +export const createHref = ( + path: string, + history: History, + options?: ICreateHrefOptions +): string => { + return options?.shouldNotCreateHref ? path : history.createHref({ pathname: path }); +}; diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.test.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.test.tsx index eba632d86dc66..82fbb8940d460 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.test.tsx @@ -5,7 +5,6 @@ */ import '../../__mocks__/kea.mock'; -import '../../__mocks__/react_router_history.mock'; import React from 'react'; import { shallow, mount } from 'enzyme'; diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx index 99314515f2734..e0aa5afdf38c1 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/eui_link.tsx @@ -6,11 +6,10 @@ import React from 'react'; import { useValues } from 'kea'; -import { useHistory } from 'react-router-dom'; import { EuiLink, EuiButton, EuiButtonProps, EuiLinkAnchorProps } from '@elastic/eui'; import { KibanaLogic } from '../../shared/kibana'; -import { letBrowserHandleEvent } from './link_events'; +import { letBrowserHandleEvent, createHref } from './'; /** * Generates either an EuiLink or EuiButton with a React-Router-ified link @@ -33,11 +32,10 @@ export const EuiReactRouterHelper: React.FC = ({ shouldNotCreateHref, children, }) => { - const history = useHistory(); - const { navigateToUrl } = useValues(KibanaLogic); + const { navigateToUrl, history } = useValues(KibanaLogic); // Generate the correct link href (with basename etc. accounted for) - const href = shouldNotCreateHref ? to : history.createHref({ pathname: to }); + const href = createHref(to, history, { shouldNotCreateHref }); const reactRouterLinkClick = (event: React.MouseEvent) => { if (onClick) onClick(); // Run any passed click events (e.g. telemetry) @@ -47,7 +45,7 @@ export const EuiReactRouterHelper: React.FC = ({ event.preventDefault(); // Perform SPA navigation. - navigateToUrl(href); + navigateToUrl(to, { shouldNotCreateHref }); }; const reactRouterProps = { href, onClick: reactRouterLinkClick }; diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/index.ts b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/index.ts index 46dc328633153..6915d3222c45c 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/index.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/react_router_helpers/index.ts @@ -5,5 +5,6 @@ */ export { letBrowserHandleEvent } from './link_events'; +export { createHref, ICreateHrefOptions } from './create_href'; export { EuiReactRouterLink as EuiLink } from './eui_link'; export { EuiReactRouterButton as EuiButton } from './eui_link';