From 388b2508b312d61f3fc480e2dc88379bc66ed9ce Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Mon, 21 Dec 2020 14:26:14 +0100 Subject: [PATCH] [Lens] Make sure Lens does not reload unnecessarily (#86092) --- .../public/react_expression_renderer.test.tsx | 38 +++++++++ .../public/react_expression_renderer.tsx | 18 +++-- .../lens/public/app_plugin/mounter.tsx | 80 +++++++++++-------- .../editor_frame/suggestion_panel.tsx | 14 +++- 4 files changed, 108 insertions(+), 42 deletions(-) diff --git a/src/plugins/expressions/public/react_expression_renderer.test.tsx b/src/plugins/expressions/public/react_expression_renderer.test.tsx index e52d4d153882f..4ebd626e70fc3 100644 --- a/src/plugins/expressions/public/react_expression_renderer.test.tsx +++ b/src/plugins/expressions/public/react_expression_renderer.test.tsx @@ -146,6 +146,44 @@ describe('ExpressionRenderer', () => { instance.unmount(); }); + it('waits for debounce period on other loader option change if specified', () => { + jest.useFakeTimers(); + + const refreshSubject = new Subject(); + const loaderUpdate = jest.fn(); + + (ExpressionLoader as jest.Mock).mockImplementation(() => { + return { + render$: new Subject(), + data$: new Subject(), + loading$: new Subject(), + update: loaderUpdate, + destroy: jest.fn(), + }; + }); + + const instance = mount( + + ); + + instance.setProps({ searchContext: { from: 'now-30m', to: 'now' } }); + + expect(loaderUpdate).toHaveBeenCalledTimes(1); + + act(() => { + jest.runAllTimers(); + }); + + expect(loaderUpdate).toHaveBeenCalledTimes(2); + + instance.unmount(); + }); + it('should display a custom error message if the user provides one and then remove it after successful render', () => { const dataSubject = new Subject(); const data$ = dataSubject.asObservable().pipe(share()); diff --git a/src/plugins/expressions/public/react_expression_renderer.tsx b/src/plugins/expressions/public/react_expression_renderer.tsx index 894325c8b65f7..eac2371ec66d0 100644 --- a/src/plugins/expressions/public/react_expression_renderer.tsx +++ b/src/plugins/expressions/public/react_expression_renderer.tsx @@ -90,21 +90,23 @@ export const ReactExpressionRenderer = ({ null ); const [debouncedExpression, setDebouncedExpression] = useState(expression); - useEffect(() => { + const [waitingForDebounceToComplete, setDebouncePending] = useState(false); + useShallowCompareEffect(() => { if (debounce === undefined) { return; } + setDebouncePending(true); const handler = setTimeout(() => { setDebouncedExpression(expression); + setDebouncePending(false); }, debounce); return () => { clearTimeout(handler); }; - }, [expression, debounce]); + }, [expression, expressionLoaderOptions, debounce]); const activeExpression = debounce !== undefined ? debouncedExpression : expression; - const waitingForDebounceToComplete = debounce !== undefined && expression !== debouncedExpression; /* eslint-disable react-hooks/exhaustive-deps */ // OK to ignore react-hooks/exhaustive-deps because options update is handled by calling .update() @@ -182,12 +184,16 @@ export const ReactExpressionRenderer = ({ // Re-fetch data automatically when the inputs change useShallowCompareEffect( () => { - if (expressionLoaderRef.current) { + // only update the loader if the debounce period is over + if (expressionLoaderRef.current && !waitingForDebounceToComplete) { expressionLoaderRef.current.update(activeExpression, expressionLoaderOptions); } }, - // when expression is changed by reference and when any other loaderOption is changed by reference - [{ activeExpression, ...expressionLoaderOptions }] + // when debounced, wait for debounce status to change to update loader. + // Otherwise, update when expression is changed by reference and when any other loaderOption is changed by reference + debounce === undefined + ? [{ activeExpression, ...expressionLoaderOptions }] + : [{ waitingForDebounceToComplete }] ); /* eslint-enable react-hooks/exhaustive-deps */ diff --git a/x-pack/plugins/lens/public/app_plugin/mounter.tsx b/x-pack/plugins/lens/public/app_plugin/mounter.tsx index b09ecfdcd5553..fbfd9c5758948 100644 --- a/x-pack/plugins/lens/public/app_plugin/mounter.tsx +++ b/x-pack/plugins/lens/public/app_plugin/mounter.tsx @@ -3,11 +3,12 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import React from 'react'; +import React, { useCallback } from 'react'; import { AppMountParameters, CoreSetup } from 'kibana/public'; import { FormattedMessage, I18nProvider } from '@kbn/i18n/react'; import { HashRouter, Route, RouteComponentProps, Switch } from 'react-router-dom'; +import { History } from 'history'; import { render, unmountComponentAtNode } from 'react-dom'; import { i18n } from '@kbn/i18n'; @@ -86,25 +87,22 @@ export async function mountApp( }) ); - const getInitialInput = ( - routeProps: RouteComponentProps<{ id?: string }>, - editByValue?: boolean - ): LensEmbeddableInput | undefined => { + const getInitialInput = (id?: string, editByValue?: boolean): LensEmbeddableInput | undefined => { if (editByValue) { return embeddableEditorIncomingState?.valueInput as LensByValueInput; } - if (routeProps.match.params.id) { - return { savedObjectId: routeProps.match.params.id } as LensByReferenceInput; + if (id) { + return { savedObjectId: id } as LensByReferenceInput; } }; - const redirectTo = (routeProps: RouteComponentProps<{ id?: string }>, savedObjectId?: string) => { + const redirectTo = (history: History, savedObjectId?: string) => { if (!savedObjectId) { - routeProps.history.push({ pathname: '/', search: routeProps.history.location.search }); + history.push({ pathname: '/', search: history.location.search }); } else { - routeProps.history.push({ + history.push({ pathname: `/edit/${savedObjectId}`, - search: routeProps.history.location.search, + search: history.location.search, }); } }; @@ -144,27 +142,45 @@ export async function mountApp( } }; - const renderEditor = ( - routeProps: RouteComponentProps<{ id?: string }>, - editByValue?: boolean + // const featureFlagConfig = await getByValueFeatureFlag(); + const EditorRenderer = React.memo( + (props: { id?: string; history: History; editByValue?: boolean }) => { + const redirectCallback = useCallback( + (id?: string) => { + redirectTo(props.history, id); + }, + [props.history] + ); + trackUiEvent('loaded'); + return ( + + ); + } + ); + + const EditorRoute = ( + routeProps: RouteComponentProps<{ id?: string }> & { editByValue?: boolean } ) => { - trackUiEvent('loaded'); return ( - redirectTo(routeProps, savedObjectId)} - redirectToOrigin={redirectToOrigin} - redirectToDashboard={redirectToDashboard} - onAppLeave={params.onAppLeave} - setHeaderActionMenu={params.setHeaderActionMenu} + ); }; @@ -185,13 +201,13 @@ export async function mountApp( - + renderEditor(routeProps, true)} + render={(routeProps) => } /> - + diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx index e42d4daffbb66..338a998b6b4dc 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx @@ -7,7 +7,7 @@ import './suggestion_panel.scss'; import _, { camelCase } from 'lodash'; -import React, { useState, useEffect, useMemo } from 'react'; +import React, { useState, useEffect, useMemo, useRef } from 'react'; import { FormattedMessage } from '@kbn/i18n/react'; import { EuiIcon, @@ -270,13 +270,19 @@ export function SuggestionPanel({ [frame.query, frame.dateRange.fromDate, frame.dateRange.toDate, frame.filters] ); + const contextRef = useRef(context); + contextRef.current = context; + const AutoRefreshExpressionRenderer = useMemo(() => { const autoRefreshFetch$ = plugins.data.query.timefilter.timefilter.getAutoRefreshFetch$(); return (props: ReactExpressionRendererProps) => ( - + ); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [plugins.data.query.timefilter.timefilter, context]); + }, [plugins.data.query.timefilter.timefilter, ExpressionRendererComponent]); const [lastSelectedSuggestion, setLastSelectedSuggestion] = useState(-1);