From ef3c8ca71fb385208284c5be0dbb32fb92a35503 Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Thu, 20 Apr 2023 17:14:03 -0400 Subject: [PATCH 01/17] store dashboard search session on instance instead of input. Debounce parent input subscription, remove session only check from should fetch logic --- .../embeddable/create/create_dashboard.ts | 5 +- ...rt_dashboard_search_session_integration.ts | 11 ++-- .../embeddable/dashboard_container.tsx | 7 +- .../state/dashboard_container_reducers.ts | 7 -- .../diffing/dashboard_diffing_functions.ts | 18 +++++- .../diffing/dashboard_diffing_integration.ts | 64 +++++++++---------- .../public/lib/embeddables/embeddable.tsx | 16 +++-- .../filterable_embeddable/should_fetch.tsx | 27 ++++---- 8 files changed, 87 insertions(+), 68 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts index 7609a4f3eb95f..09f98f42b4d99 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts @@ -211,6 +211,7 @@ export const createDashboard = async ( // -------------------------------------------------------------------------------------- // Set up search sessions integration. // -------------------------------------------------------------------------------------- + let initialSearchSessionId; if (searchSessionSettings) { const { sessionIdToRestore } = searchSessionSettings; @@ -223,7 +224,7 @@ export const createDashboard = async ( } const existingSession = session.getSessionId(); - const initialSearchSessionId = + initialSearchSessionId = sessionIdToRestore ?? (existingSession && incomingEmbeddable ? existingSession : session.start()); @@ -232,7 +233,6 @@ export const createDashboard = async ( creationOptions?.searchSessionSettings ); }); - initialInput.searchSessionId = initialSearchSessionId; } // -------------------------------------------------------------------------------------- @@ -278,6 +278,7 @@ export const createDashboard = async ( const dashboardContainer = new DashboardContainer( initialInput, reduxEmbeddablePackage, + initialSearchSessionId, savedObjectResult?.dashboardInput, dashboardCreationStartTime, undefined, diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/search_sessions/start_dashboard_search_session_integration.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/search_sessions/start_dashboard_search_session_integration.ts index 506083ab25386..7f59b56c228b6 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/search_sessions/start_dashboard_search_session_integration.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/search_sessions/start_dashboard_search_session_integration.ts @@ -6,14 +6,13 @@ * Side Public License, v 1. */ -import { debounceTime, pairwise, skip } from 'rxjs/operators'; +import { pairwise, skip } from 'rxjs/operators'; import { noSearchSessionStorageCapabilityMessage } from '@kbn/data-plugin/public'; import { DashboardContainer } from '../../dashboard_container'; import { DashboardContainerInput } from '../../../../../common'; import { pluginServices } from '../../../../services/plugin_services'; -import { CHANGE_CHECK_DEBOUNCE } from '../../../../dashboard_constants'; import { DashboardCreationOptions } from '../../dashboard_container_factory'; import { getShouldRefresh } from '../../../state/diffing/dashboard_diffing_integration'; @@ -57,10 +56,10 @@ export function startDashboardSearchSessionIntegration( // listen to and compare states to determine when to launch a new session. this.getInput$() - .pipe(pairwise(), debounceTime(CHANGE_CHECK_DEBOUNCE)) - .subscribe(async (states) => { + .pipe(pairwise()) + .subscribe((states) => { const [previous, current] = states as DashboardContainerInput[]; - const shouldRefetch = await getShouldRefresh.bind(this)(previous, current); + const shouldRefetch = getShouldRefresh.bind(this)(previous, current); if (!shouldRefetch) return; const currentSearchSessionId = this.getState().explicitInput.searchSessionId; @@ -83,7 +82,7 @@ export function startDashboardSearchSessionIntegration( })(); if (updatedSearchSessionId && updatedSearchSessionId !== currentSearchSessionId) { - this.dispatch.setSearchSessionId(updatedSearchSessionId); + this.searchSessionId = updatedSearchSessionId; } }); diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx index 5b7a589afa950..6ee260dab90e8 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx @@ -95,6 +95,8 @@ export class DashboardContainer extends Container void; private cleanupStateTools: () => void; @@ -117,6 +119,7 @@ export class DashboardContainer extends Container - ) => { - state.explicitInput.searchSessionId = action.payload; - }, - // ------------------------------------------------------------------------------ // Unsaved Changes Reducers // ------------------------------------------------------------------------------ diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts index 7f2a55044b527..534a243bba02b 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts @@ -37,7 +37,7 @@ export type DashboardDiffFunctions = { ) => boolean | Promise; }; -export const isKeyEqual = async ( +export const isKeyEqualAsync = async ( key: keyof DashboardContainerInput, diffFunctionProps: DiffFunctionProps, diffingFunctions: DashboardDiffFunctions @@ -52,6 +52,22 @@ export const isKeyEqual = async ( return fastIsEqual(diffFunctionProps.currentValue, diffFunctionProps.lastValue); }; +export const isKeyEqual = ( + key: keyof Omit, // only Panels is async + diffFunctionProps: DiffFunctionProps, + diffingFunctions: DashboardDiffFunctions +) => { + const propsAsNever = diffFunctionProps as never; // todo figure out why props has conflicting types in some constituents. + const diffingFunction = diffingFunctions[key]; + if (!diffingFunction) { + return fastIsEqual(diffFunctionProps.currentValue, diffFunctionProps.lastValue); + } + if (diffingFunction?.prototype?.name === 'AsyncFunction') throw new Error('The function for key'); + if (diffingFunction) { + return diffingFunction(propsAsNever); + } +}; + /** * A collection of functions which diff individual keys of dashboard state. If a key is missing from this list it is * diffed by the default diffing function, fastIsEqual. diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts index 02399543750cd..3f2c00ca3d2e2 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts @@ -9,13 +9,13 @@ import { omit } from 'lodash'; import { AnyAction, Middleware } from 'redux'; import { debounceTime, Observable, Subject, switchMap } from 'rxjs'; -import { DashboardContainerInput } from '../../../../common'; -import type { DashboardDiffFunctions } from './dashboard_diffing_functions'; import { isKeyEqual, + isKeyEqualAsync, shouldRefreshDiffingFunctions, unsavedChangesDiffingFunctions, } from './dashboard_diffing_functions'; +import { DashboardContainerInput } from '../../../../common'; import { pluginServices } from '../../../services/plugin_services'; import { DashboardContainer, DashboardCreationOptions } from '../..'; import { CHANGE_CHECK_DEBOUNCE } from '../../../dashboard_constants'; @@ -64,7 +64,7 @@ export const keysNotConsideredUnsavedChanges: Array = [ +const sessionChangeKeys: Array> = [ 'query', 'filters', 'timeRange', @@ -138,42 +138,17 @@ export async function getUnsavedChanges( const allKeys = [...new Set([...Object.keys(lastInput), ...Object.keys(input)])] as Array< keyof DashboardContainerInput >; - return await getInputChanges(this, lastInput, input, allKeys, unsavedChangesDiffingFunctions); -} - -export async function getShouldRefresh( - this: DashboardContainer, - lastInput: DashboardContainerInput, - input: DashboardContainerInput -): Promise { - const inputChanges = await getInputChanges( - this, - lastInput, - input, - refetchKeys, - shouldRefreshDiffingFunctions - ); - return Object.keys(inputChanges).length > 0; -} - -async function getInputChanges( - container: DashboardContainer, - lastInput: DashboardContainerInput, - input: DashboardContainerInput, - keys: Array, - diffingFunctions: DashboardDiffFunctions -): Promise> { - const keyComparePromises = keys.map( + const keyComparePromises = allKeys.map( (key) => new Promise<{ key: keyof DashboardContainerInput; isEqual: boolean }>((resolve) => { if (input[key] === undefined && lastInput[key] === undefined) { resolve({ key, isEqual: true }); } - isKeyEqual( + isKeyEqualAsync( key, { - container, + container: this, currentValue: input[key], currentInput: input, @@ -181,7 +156,7 @@ async function getInputChanges( lastValue: lastInput[key], lastInput, }, - diffingFunctions + unsavedChangesDiffingFunctions ).then((isEqual) => resolve({ key, isEqual })); }) ); @@ -195,6 +170,31 @@ async function getInputChanges( return inputChanges; } +export function getShouldRefresh( + this: DashboardContainer, + lastInput: DashboardContainerInput, + input: DashboardContainerInput +): boolean { + for (const refetchKey of sessionChangeKeys) { + if ( + !isKeyEqual( + refetchKey, + { + container: this, + currentValue: input[refetchKey], + currentInput: input, + lastValue: lastInput[refetchKey], + lastInput, + }, + shouldRefreshDiffingFunctions + ) + ) { + return false; + } + } + return true; +} + function updateUnsavedChangesState( this: DashboardContainer, unsavedChanges: Partial diff --git a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx index 75c666d4e2903..d3226aa5f9024 100644 --- a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx +++ b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx @@ -86,13 +86,15 @@ export abstract class Embeddable< this.outputSubject.next(this.output); if (parent) { - this.parentSubscription = Rx.merge(parent.getInput$(), parent.getOutput$()).subscribe(() => { - // Make sure this panel hasn't been removed immediately after it was added, but before it finished loading. - if (!parent.getInput().panels[this.id]) return; - - const newInput = parent.getInputForChild(this.id); - this.onResetInput(newInput); - }); + this.parentSubscription = Rx.merge(parent.getInput$(), parent.getOutput$()) + .pipe(Rx.debounceTime(0)) + .subscribe(() => { + // Make sure this panel hasn't been removed immediately after it was added, but before it finished loading. + if (!parent.getInput().panels[this.id]) return; + + const newInput = parent.getInputForChild(this.id); + this.onResetInput(newInput); + }); } this.getOutput$() diff --git a/src/plugins/embeddable/public/lib/filterable_embeddable/should_fetch.tsx b/src/plugins/embeddable/public/lib/filterable_embeddable/should_fetch.tsx index b8b9fb1f4795f..68d9df23bb612 100644 --- a/src/plugins/embeddable/public/lib/filterable_embeddable/should_fetch.tsx +++ b/src/plugins/embeddable/public/lib/filterable_embeddable/should_fetch.tsx @@ -27,19 +27,24 @@ export function shouldFetch$< return updated$.pipe(map(() => getInput())).pipe( // wrapping distinctUntilChanged with startWith and skip to prime distinctUntilChanged with an initial input value. startWith(getInput()), - distinctUntilChanged((a: TFilterableEmbeddableInput, b: TFilterableEmbeddableInput) => { - // Only need to diff searchSessionId when container uses search sessions because - // searchSessionId changes with any filter, query, or time changes - if (a.searchSessionId !== undefined || b.searchSessionId !== undefined) { - return a.searchSessionId === b.searchSessionId; - } + distinctUntilChanged( + (previous: TFilterableEmbeddableInput, current: TFilterableEmbeddableInput) => { + if ( + !fastIsEqual( + [previous.searchSessionId, previous.query, previous.timeRange, previous.timeslice], + [current.searchSessionId, current.query, current.timeRange, current.timeslice] + ) + ) { + return false; + } - if (!fastIsEqual([a.query, a.timeRange, a.timeslice], [b.query, b.timeRange, b.timeslice])) { - return false; + return onlyDisabledFiltersChanged( + previous.filters, + current.filters, + shouldRefreshFilterCompareOptions + ); } - - return onlyDisabledFiltersChanged(a.filters, b.filters, shouldRefreshFilterCompareOptions); - }), + ), skip(1) ); } From 0b8aa958b1e4b7d09bd750fc0671f8c942ec316b Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 10:57:55 -0600 Subject: [PATCH 02/17] type checks --- .../state/diffing/dashboard_diffing_integration.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts index 3f2c00ca3d2e2..592f75c986eab 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts @@ -29,7 +29,6 @@ import { dashboardContainerReducers } from '../dashboard_container_reducers'; export const reducersToIgnore: Array = [ 'setTimeslice', 'setFullScreenMode', - 'setSearchSessionId', 'setExpandedPanelId', 'setHasUnsavedChanges', ]; @@ -40,7 +39,6 @@ export const reducersToIgnore: Array = const keysToOmitFromSessionStorage: Array = [ 'lastReloadRequestTime', 'executionContext', - 'searchSessionId', 'timeslice', 'id', @@ -55,7 +53,6 @@ const keysToOmitFromSessionStorage: Array = [ export const keysNotConsideredUnsavedChanges: Array = [ 'lastReloadRequestTime', 'executionContext', - 'searchSessionId', 'timeslice', 'viewMode', 'id', From a9315156481e60af7db380dad394ff31e989e790 Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 12:37:05 -0600 Subject: [PATCH 03/17] fix getShouldRefresh --- .../diffing/dashboard_diffing_functions.ts | 7 +- .../diffing/dashboard_diffing_integration.ts | 12 +- .../public/lib/embeddables/embeddable.tsx | 133 +++++------------- 3 files changed, 46 insertions(+), 106 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts index 534a243bba02b..4f00f982da14d 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts @@ -62,10 +62,11 @@ export const isKeyEqual = ( if (!diffingFunction) { return fastIsEqual(diffFunctionProps.currentValue, diffFunctionProps.lastValue); } - if (diffingFunction?.prototype?.name === 'AsyncFunction') throw new Error('The function for key'); - if (diffingFunction) { - return diffingFunction(propsAsNever); + + if (diffingFunction?.prototype?.name === 'AsyncFunction') { + throw new Error(`The function for key "${key}" is async, must use isKeyEqualAsync for asynchronous functions`); } + return diffingFunction(propsAsNever); }; /** diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts index 592f75c986eab..feb294ebbcd27 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts @@ -172,24 +172,24 @@ export function getShouldRefresh( lastInput: DashboardContainerInput, input: DashboardContainerInput ): boolean { - for (const refetchKey of sessionChangeKeys) { + for (const key of sessionChangeKeys) { if ( !isKeyEqual( - refetchKey, + key, { container: this, - currentValue: input[refetchKey], + currentValue: input[key], currentInput: input, - lastValue: lastInput[refetchKey], + lastValue: lastInput[key], lastInput, }, shouldRefreshDiffingFunctions ) ) { - return false; + return true; } } - return true; + return false; } function updateUnsavedChangesState( diff --git a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx index d3226aa5f9024..de1a723590683 100644 --- a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx +++ b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx @@ -6,32 +6,24 @@ * Side Public License, v 1. */ -import fastIsEqual from 'fast-deep-equal'; -import { cloneDeep } from 'lodash'; +import { cloneDeep, isEqual } from 'lodash'; import * as Rx from 'rxjs'; import { merge } from 'rxjs'; import { debounceTime, distinctUntilChanged, map, skip } from 'rxjs/operators'; -import { RenderCompleteDispatcher } from '@kbn/kibana-utils-plugin/public'; +import { RenderCompleteDispatcher } from '../../../../kibana_utils/public'; import { Adapters } from '../types'; import { IContainer } from '../containers'; -import { EmbeddableError, EmbeddableOutput, IEmbeddable } from './i_embeddable'; +import { EmbeddableOutput, IEmbeddable } from './i_embeddable'; import { EmbeddableInput, ViewMode } from '../../../common/types'; -import { genericEmbeddableInputIsEqual, omitGenericEmbeddableInput } from './diff_embeddable_input'; function getPanelTitle(input: EmbeddableInput, output: EmbeddableOutput) { - if (input.hidePanelTitles) return ''; - return input.title ?? output.defaultTitle; -} -function getPanelDescription(input: EmbeddableInput, output: EmbeddableOutput) { - if (input.hidePanelTitles) return ''; - return input.description ?? output.defaultDescription; + return input.hidePanelTitles ? '' : input.title === undefined ? output.defaultTitle : input.title; } export abstract class Embeddable< TEmbeddableInput extends EmbeddableInput = EmbeddableInput, - TEmbeddableOutput extends EmbeddableOutput = EmbeddableOutput, - TNode = any -> implements IEmbeddable + TEmbeddableOutput extends EmbeddableOutput = EmbeddableOutput +> implements IEmbeddable { static runtimeId: number = 0; @@ -40,8 +32,6 @@ export abstract class Embeddable< public readonly parent?: IContainer; public readonly isContainer: boolean = false; public readonly deferEmbeddableLoad: boolean = false; - public catchError?(error: EmbeddableError, domNode: HTMLElement | Element): TNode | (() => void); - public abstract readonly type: string; public readonly id: string; public fatalError?: Error; @@ -49,10 +39,8 @@ export abstract class Embeddable< protected output: TEmbeddableOutput; protected input: TEmbeddableInput; - private readonly inputSubject = new Rx.ReplaySubject(1); - private readonly outputSubject = new Rx.ReplaySubject(1); - private readonly input$ = this.inputSubject.asObservable(); - private readonly output$ = this.outputSubject.asObservable(); + private readonly input$: Rx.BehaviorSubject; + private readonly output$: Rx.BehaviorSubject; protected renderComplete = new RenderCompleteDispatcher(); @@ -64,16 +52,8 @@ export abstract class Embeddable< constructor(input: TEmbeddableInput, output: TEmbeddableOutput, parent?: IContainer) { this.id = input.id; - this.output = { title: getPanelTitle(input, output), - description: getPanelDescription(input, output), - ...(this.reportsEmbeddableLoad() - ? {} - : { - loading: false, - rendered: true, - }), ...output, }; this.input = { @@ -82,19 +62,17 @@ export abstract class Embeddable< }; this.parent = parent; - this.inputSubject.next(this.input); - this.outputSubject.next(this.output); + this.input$ = new Rx.BehaviorSubject(this.input); + this.output$ = new Rx.BehaviorSubject(this.output); if (parent) { - this.parentSubscription = Rx.merge(parent.getInput$(), parent.getOutput$()) - .pipe(Rx.debounceTime(0)) - .subscribe(() => { - // Make sure this panel hasn't been removed immediately after it was added, but before it finished loading. - if (!parent.getInput().panels[this.id]) return; - - const newInput = parent.getInputForChild(this.id); - this.onResetInput(newInput); - }); + this.parentSubscription = Rx.merge(parent.getInput$(), parent.getOutput$()).subscribe(() => { + // Make sure this panel hasn't been removed immediately after it was added, but before it finished loading. + if (!parent.getInput().panels[this.id]) return; + + const newInput = parent.getInputForChild(this.id); + this.onResetInput(newInput); + }); } this.getOutput$() @@ -102,20 +80,12 @@ export abstract class Embeddable< map(({ title }) => title || ''), distinctUntilChanged() ) - .subscribe((title) => this.renderComplete.setTitle(title)); - } - - public reportsEmbeddableLoad() { - return false; - } - - public refreshInputFromParent() { - if (!this.parent) return; - // Make sure this panel hasn't been removed immediately after it was added, but before it finished loading. - if (!this.parent.getInput().panels[this.id]) return; - - const newInput = this.parent.getInputForChild(this.id); - this.onResetInput(newInput); + .subscribe( + (title) => { + this.renderComplete.setTitle(title); + }, + () => {} + ); } public getIsContainer(): this is IContainer { @@ -150,54 +120,23 @@ export abstract class Embeddable< } public getInput$(): Readonly> { - return this.input$; + return this.input$.asObservable(); } public getOutput$(): Readonly> { - return this.output$; + return this.output$.asObservable(); } public getOutput(): Readonly { return this.output; } - public async getExplicitInputIsEqual( - lastExplicitInput: Partial - ): Promise { - const currentExplicitInput = this.getExplicitInput(); - return ( - genericEmbeddableInputIsEqual(lastExplicitInput, currentExplicitInput) && - fastIsEqual( - omitGenericEmbeddableInput(lastExplicitInput), - omitGenericEmbeddableInput(currentExplicitInput) - ) - ); - } - - public getExplicitInput() { - const root = this.getRoot(); - if (root.getIsContainer()) { - return ( - (root.getInput().panels?.[this.id]?.explicitInput as TEmbeddableInput) ?? this.getInput() - ); - } - return this.getInput(); - } - - public getPersistableInput() { - return this.getExplicitInput(); - } - public getInput(): Readonly { return this.input; } public getTitle(): string { - return this.output.title ?? ''; - } - - public getDescription(): string { - return this.output.description ?? ''; + return this.output.title || ''; } /** @@ -224,13 +163,14 @@ export abstract class Embeddable< } } - public render(el: HTMLElement): TNode | void { + public render(el: HTMLElement): void { this.renderComplete.setEl(el); this.renderComplete.setTitle(this.output.title || ''); if (this.destroyed) { throw new Error('Embeddable has been destroyed'); } + return; } /** @@ -249,8 +189,8 @@ export abstract class Embeddable< public destroy(): void { this.destroyed = true; - this.inputSubject.complete(); - this.outputSubject.complete(); + this.input$.complete(); + this.output$.complete(); if (this.parentSubscription) { this.parentSubscription.unsubscribe(); @@ -268,20 +208,20 @@ export abstract class Embeddable< } } - public updateOutput(outputChanges: Partial): void { + protected updateOutput(outputChanges: Partial): void { const newOutput = { ...this.output, ...outputChanges, }; - if (!fastIsEqual(this.output, newOutput)) { + if (!isEqual(this.output, newOutput)) { this.output = newOutput; - this.outputSubject.next(this.output); + this.output$.next(this.output); } } protected onFatalError(e: Error) { this.fatalError = e; - this.outputSubject.error(e); + this.output$.error(e); // if the container is waiting for this embeddable to complete loading, // a fatal error counts as complete. if (this.deferEmbeddableLoad && this.parent?.isContainer) { @@ -290,13 +230,12 @@ export abstract class Embeddable< } private onResetInput(newInput: TEmbeddableInput) { - if (!fastIsEqual(this.input, newInput)) { + if (!isEqual(this.input, newInput)) { const oldLastReloadRequestTime = this.input.lastReloadRequestTime; this.input = newInput; - this.inputSubject.next(newInput); + this.input$.next(newInput); this.updateOutput({ title: getPanelTitle(this.input, this.output), - description: getPanelDescription(this.input, this.output), } as Partial); if (oldLastReloadRequestTime !== newInput.lastReloadRequestTime) { this.reload(); From 7d0c8a8a4bbde18852e763dfc858e3455b5a9baa Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 12:38:51 -0600 Subject: [PATCH 04/17] revert changes to embeddable.tsx --- .../public/lib/embeddables/embeddable.tsx | 119 +++++++++++++----- 1 file changed, 89 insertions(+), 30 deletions(-) diff --git a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx index de1a723590683..6f5a843fa890b 100644 --- a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx +++ b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx @@ -6,24 +6,32 @@ * Side Public License, v 1. */ -import { cloneDeep, isEqual } from 'lodash'; +import fastIsEqual from 'fast-deep-equal'; +import { cloneDeep } from 'lodash'; import * as Rx from 'rxjs'; import { merge } from 'rxjs'; import { debounceTime, distinctUntilChanged, map, skip } from 'rxjs/operators'; -import { RenderCompleteDispatcher } from '../../../../kibana_utils/public'; +import { RenderCompleteDispatcher } from '@kbn/kibana-utils-plugin/public'; import { Adapters } from '../types'; import { IContainer } from '../containers'; -import { EmbeddableOutput, IEmbeddable } from './i_embeddable'; +import { EmbeddableError, EmbeddableOutput, IEmbeddable } from './i_embeddable'; import { EmbeddableInput, ViewMode } from '../../../common/types'; +import { genericEmbeddableInputIsEqual, omitGenericEmbeddableInput } from './diff_embeddable_input'; function getPanelTitle(input: EmbeddableInput, output: EmbeddableOutput) { - return input.hidePanelTitles ? '' : input.title === undefined ? output.defaultTitle : input.title; + if (input.hidePanelTitles) return ''; + return input.title ?? output.defaultTitle; +} +function getPanelDescription(input: EmbeddableInput, output: EmbeddableOutput) { + if (input.hidePanelTitles) return ''; + return input.description ?? output.defaultDescription; } export abstract class Embeddable< TEmbeddableInput extends EmbeddableInput = EmbeddableInput, - TEmbeddableOutput extends EmbeddableOutput = EmbeddableOutput -> implements IEmbeddable + TEmbeddableOutput extends EmbeddableOutput = EmbeddableOutput, + TNode = any +> implements IEmbeddable { static runtimeId: number = 0; @@ -32,6 +40,8 @@ export abstract class Embeddable< public readonly parent?: IContainer; public readonly isContainer: boolean = false; public readonly deferEmbeddableLoad: boolean = false; + public catchError?(error: EmbeddableError, domNode: HTMLElement | Element): TNode | (() => void); + public abstract readonly type: string; public readonly id: string; public fatalError?: Error; @@ -39,8 +49,10 @@ export abstract class Embeddable< protected output: TEmbeddableOutput; protected input: TEmbeddableInput; - private readonly input$: Rx.BehaviorSubject; - private readonly output$: Rx.BehaviorSubject; + private readonly inputSubject = new Rx.ReplaySubject(1); + private readonly outputSubject = new Rx.ReplaySubject(1); + private readonly input$ = this.inputSubject.asObservable(); + private readonly output$ = this.outputSubject.asObservable(); protected renderComplete = new RenderCompleteDispatcher(); @@ -52,8 +64,16 @@ export abstract class Embeddable< constructor(input: TEmbeddableInput, output: TEmbeddableOutput, parent?: IContainer) { this.id = input.id; + this.output = { title: getPanelTitle(input, output), + description: getPanelDescription(input, output), + ...(this.reportsEmbeddableLoad() + ? {} + : { + loading: false, + rendered: true, + }), ...output, }; this.input = { @@ -62,8 +82,8 @@ export abstract class Embeddable< }; this.parent = parent; - this.input$ = new Rx.BehaviorSubject(this.input); - this.output$ = new Rx.BehaviorSubject(this.output); + this.inputSubject.next(this.input); + this.outputSubject.next(this.output); if (parent) { this.parentSubscription = Rx.merge(parent.getInput$(), parent.getOutput$()).subscribe(() => { @@ -80,12 +100,20 @@ export abstract class Embeddable< map(({ title }) => title || ''), distinctUntilChanged() ) - .subscribe( - (title) => { - this.renderComplete.setTitle(title); - }, - () => {} - ); + .subscribe((title) => this.renderComplete.setTitle(title)); + } + + public reportsEmbeddableLoad() { + return false; + } + + public refreshInputFromParent() { + if (!this.parent) return; + // Make sure this panel hasn't been removed immediately after it was added, but before it finished loading. + if (!this.parent.getInput().panels[this.id]) return; + + const newInput = this.parent.getInputForChild(this.id); + this.onResetInput(newInput); } public getIsContainer(): this is IContainer { @@ -120,23 +148,54 @@ export abstract class Embeddable< } public getInput$(): Readonly> { - return this.input$.asObservable(); + return this.input$; } public getOutput$(): Readonly> { - return this.output$.asObservable(); + return this.output$; } public getOutput(): Readonly { return this.output; } + public async getExplicitInputIsEqual( + lastExplicitInput: Partial + ): Promise { + const currentExplicitInput = this.getExplicitInput(); + return ( + genericEmbeddableInputIsEqual(lastExplicitInput, currentExplicitInput) && + fastIsEqual( + omitGenericEmbeddableInput(lastExplicitInput), + omitGenericEmbeddableInput(currentExplicitInput) + ) + ); + } + + public getExplicitInput() { + const root = this.getRoot(); + if (root.getIsContainer()) { + return ( + (root.getInput().panels?.[this.id]?.explicitInput as TEmbeddableInput) ?? this.getInput() + ); + } + return this.getInput(); + } + + public getPersistableInput() { + return this.getExplicitInput(); + } + public getInput(): Readonly { return this.input; } public getTitle(): string { - return this.output.title || ''; + return this.output.title ?? ''; + } + + public getDescription(): string { + return this.output.description ?? ''; } /** @@ -163,14 +222,13 @@ export abstract class Embeddable< } } - public render(el: HTMLElement): void { + public render(el: HTMLElement): TNode | void { this.renderComplete.setEl(el); this.renderComplete.setTitle(this.output.title || ''); if (this.destroyed) { throw new Error('Embeddable has been destroyed'); } - return; } /** @@ -189,8 +247,8 @@ export abstract class Embeddable< public destroy(): void { this.destroyed = true; - this.input$.complete(); - this.output$.complete(); + this.inputSubject.complete(); + this.outputSubject.complete(); if (this.parentSubscription) { this.parentSubscription.unsubscribe(); @@ -208,20 +266,20 @@ export abstract class Embeddable< } } - protected updateOutput(outputChanges: Partial): void { + public updateOutput(outputChanges: Partial): void { const newOutput = { ...this.output, ...outputChanges, }; - if (!isEqual(this.output, newOutput)) { + if (!fastIsEqual(this.output, newOutput)) { this.output = newOutput; - this.output$.next(this.output); + this.outputSubject.next(this.output); } } protected onFatalError(e: Error) { this.fatalError = e; - this.output$.error(e); + this.outputSubject.error(e); // if the container is waiting for this embeddable to complete loading, // a fatal error counts as complete. if (this.deferEmbeddableLoad && this.parent?.isContainer) { @@ -230,12 +288,13 @@ export abstract class Embeddable< } private onResetInput(newInput: TEmbeddableInput) { - if (!isEqual(this.input, newInput)) { + if (!fastIsEqual(this.input, newInput)) { const oldLastReloadRequestTime = this.input.lastReloadRequestTime; this.input = newInput; - this.input$.next(newInput); + this.inputSubject.next(newInput); this.updateOutput({ title: getPanelTitle(this.input, this.output), + description: getPanelDescription(this.input, this.output), } as Partial); if (oldLastReloadRequestTime !== newInput.lastReloadRequestTime) { this.reload(); @@ -255,4 +314,4 @@ export abstract class Embeddable< public supportedTriggers(): string[] { return []; } -} +} \ No newline at end of file From bffbb0072e266b4cc04e9e62511221d1154f9bce Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Mon, 24 Apr 2023 18:44:03 +0000 Subject: [PATCH 05/17] [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' --- src/plugins/embeddable/public/lib/embeddables/embeddable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx index 6f5a843fa890b..75c666d4e2903 100644 --- a/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx +++ b/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx @@ -314,4 +314,4 @@ export abstract class Embeddable< public supportedTriggers(): string[] { return []; } -} \ No newline at end of file +} From e7102087a88d8fe63646cb634b4438cc61b6d070 Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 13:12:16 -0600 Subject: [PATCH 06/17] remove searchSessionId test since its no longer in input --- .../embeddable/dashboard_container.test.tsx | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx index bdce2754ba0dd..4b5ef7bfb743a 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx @@ -169,25 +169,6 @@ test('Container view mode change propagates to new children', async () => { expect(embeddable.getInput().viewMode).toBe(ViewMode.EDIT); }); -test('searchSessionId propagates to children', async () => { - const searchSessionId1 = 'searchSessionId1'; - const container = buildMockDashboard({ searchSessionId: searchSessionId1 }); - const embeddable = await container.addNewEmbeddable< - ContactCardEmbeddableInput, - ContactCardEmbeddableOutput, - ContactCardEmbeddable - >(CONTACT_CARD_EMBEDDABLE, { - firstName: 'Bob', - }); - - expect(embeddable.getInput().searchSessionId).toBe(searchSessionId1); - - const searchSessionId2 = 'searchSessionId2'; - container.updateInput({ searchSessionId: searchSessionId2 }); - - expect(embeddable.getInput().searchSessionId).toBe(searchSessionId2); -}); - test('DashboardContainer in edit mode shows edit mode actions', async () => { const uiActionsSetup = uiActionsPluginMock.createSetupContract(); From dbd7286eb23197b1c9873694f215c1e95aac010f Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 15:46:41 -0600 Subject: [PATCH 07/17] add jest test to ensure searchSessionId updates reflected in embeddable parent updates --- .../create/create_dashboard.test.ts | 61 +++++++++++++++++++ .../embeddable/dashboard_container.test.tsx | 18 +++++- 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts index 6f9f40745322b..9c8d738258720 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts @@ -7,6 +7,7 @@ */ import { + ContactCardEmbeddableFactory, ContactCardEmbeddableInput, CONTACT_CARD_EMBEDDABLE, } from '@kbn/embeddable-plugin/public/lib/test_samples'; @@ -253,3 +254,63 @@ test('creates a control group from the control group factory and waits for it to ); expect(mockControlGroupContainer.untilInitialized).toHaveBeenCalled(); }); + +/* + * dashboard.getInput$() subscriptions are used to update: + * 1) dashboard instance searchSessionId state + * 2) child input on parent input changes + * + * Rxjs subscriptions are executioned in the order that they are created. + * This test ensures that searchSessionId update subscription is created before child input subscription + * to ensure child input subscription includes updated searchSessionId. + */ +test('searchSessionId is updated prior to child embeddable parent subscription execution', async () => { + const embeddableFactory = { + create: new ContactCardEmbeddableFactory((() => null) as any, {} as any), + getDefaultInput: jest.fn().mockResolvedValue({ + timeRange: { + to: 'now', + from: 'now-15m', + } + }), + }; + pluginServices.getServices().embeddable.getEmbeddableFactory = jest + .fn() + .mockReturnValue(embeddableFactory); + let sessionCount = 0; + pluginServices.getServices().data.search.session.start = () => { + sessionCount++; + return `searchSessionId${sessionCount}` + } + const dashboard = await createDashboard(embeddableId, { + searchSessionSettings: { + getSearchSessionIdFromURL: () => undefined, + removeSessionIdFromUrl: () => {}, + createSessionRestorationDataProvider: () => {}, + } + }); + const embeddable = await dashboard.addNewEmbeddable< + ContactCardEmbeddableInput, + ContactCardEmbeddableOutput, + ContactCardEmbeddable + >(CONTACT_CARD_EMBEDDABLE, { + firstName: 'Bob', + }); + + expect(embeddable.getInput().searchSessionId).toBe('searchSessionId1'); + + dashboard.updateInput({ + timeRange: { + to: 'now', + from: 'now-7d', + } + }); + + const embeddableInput = embeddable.getInput(); + expect(embeddableInput.timeRange).toEqual({ + to: 'now', + from: 'now-7d', + }); + expect(embeddableInput.searchSessionId).toBe('searchSessionId2'); +}); + diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx index 4b5ef7bfb743a..a5e71a4cb1e6a 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx @@ -9,6 +9,7 @@ import React from 'react'; import { mount } from 'enzyme'; +import { mockedReduxEmbeddablePackage } from '@kbn/presentation-util-plugin/public/mocks'; import { findTestSubject, nextTick } from '@kbn/test-jest-helpers'; import { I18nProvider } from '@kbn/i18n-react'; import { @@ -29,9 +30,10 @@ import { applicationServiceMock, coreMock } from '@kbn/core/public/mocks'; import { uiActionsPluginMock } from '@kbn/ui-actions-plugin/public/mocks'; import { createEditModeActionDefinition } from '@kbn/embeddable-plugin/public/lib/test_samples'; -import { buildMockDashboard, getSampleDashboardPanel } from '../../mocks'; +import { buildMockDashboard, getSampleDashboardInput, getSampleDashboardPanel } from '../../mocks'; import { pluginServices } from '../../services/plugin_services'; import { ApplicationStart } from '@kbn/core-application-browser'; +import { DashboardContainer } from './dashboard_container'; const theme = coreMock.createStart().theme; let application: ApplicationStart | undefined; @@ -169,6 +171,20 @@ test('Container view mode change propagates to new children', async () => { expect(embeddable.getInput().viewMode).toBe(ViewMode.EDIT); }); +test('searchSessionId propagates to children', async () => { + const searchSessionId1 = 'searchSessionId1'; + const container = new DashboardContainer(getSampleDashboardInput(), mockedReduxEmbeddablePackage, searchSessionId1); + const embeddable = await container.addNewEmbeddable< + ContactCardEmbeddableInput, + ContactCardEmbeddableOutput, + ContactCardEmbeddable + >(CONTACT_CARD_EMBEDDABLE, { + firstName: 'Bob', + }); + + expect(embeddable.getInput().searchSessionId).toBe(searchSessionId1); +}); + test('DashboardContainer in edit mode shows edit mode actions', async () => { const uiActionsSetup = uiActionsPluginMock.createSetupContract(); From b93d24a84e04a332f6b181a7b615e25381e648fa Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Mon, 24 Apr 2023 21:52:08 +0000 Subject: [PATCH 08/17] [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' --- .../embeddable/create/create_dashboard.test.ts | 15 +++++++-------- .../embeddable/dashboard_container.test.tsx | 6 +++++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts index 9c8d738258720..264e096ea533a 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts @@ -259,9 +259,9 @@ test('creates a control group from the control group factory and waits for it to * dashboard.getInput$() subscriptions are used to update: * 1) dashboard instance searchSessionId state * 2) child input on parent input changes - * + * * Rxjs subscriptions are executioned in the order that they are created. - * This test ensures that searchSessionId update subscription is created before child input subscription + * This test ensures that searchSessionId update subscription is created before child input subscription * to ensure child input subscription includes updated searchSessionId. */ test('searchSessionId is updated prior to child embeddable parent subscription execution', async () => { @@ -271,7 +271,7 @@ test('searchSessionId is updated prior to child embeddable parent subscription e timeRange: { to: 'now', from: 'now-15m', - } + }, }), }; pluginServices.getServices().embeddable.getEmbeddableFactory = jest @@ -280,14 +280,14 @@ test('searchSessionId is updated prior to child embeddable parent subscription e let sessionCount = 0; pluginServices.getServices().data.search.session.start = () => { sessionCount++; - return `searchSessionId${sessionCount}` - } + return `searchSessionId${sessionCount}`; + }; const dashboard = await createDashboard(embeddableId, { searchSessionSettings: { getSearchSessionIdFromURL: () => undefined, removeSessionIdFromUrl: () => {}, createSessionRestorationDataProvider: () => {}, - } + }, }); const embeddable = await dashboard.addNewEmbeddable< ContactCardEmbeddableInput, @@ -303,7 +303,7 @@ test('searchSessionId is updated prior to child embeddable parent subscription e timeRange: { to: 'now', from: 'now-7d', - } + }, }); const embeddableInput = embeddable.getInput(); @@ -313,4 +313,3 @@ test('searchSessionId is updated prior to child embeddable parent subscription e }); expect(embeddableInput.searchSessionId).toBe('searchSessionId2'); }); - diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx index a5e71a4cb1e6a..5a360446f03a8 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx @@ -173,7 +173,11 @@ test('Container view mode change propagates to new children', async () => { test('searchSessionId propagates to children', async () => { const searchSessionId1 = 'searchSessionId1'; - const container = new DashboardContainer(getSampleDashboardInput(), mockedReduxEmbeddablePackage, searchSessionId1); + const container = new DashboardContainer( + getSampleDashboardInput(), + mockedReduxEmbeddablePackage, + searchSessionId1 + ); const embeddable = await container.addNewEmbeddable< ContactCardEmbeddableInput, ContactCardEmbeddableOutput, From a19c1b350c68770de1ea1a8bc7712c62ff12e0ad Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 16:24:26 -0600 Subject: [PATCH 09/17] tslint --- .../embeddable/create/create_dashboard.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts index 264e096ea533a..c5ec696c8ad0c 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts @@ -7,8 +7,10 @@ */ import { + ContactCardEmbeddable, ContactCardEmbeddableFactory, ContactCardEmbeddableInput, + ContactCardEmbeddableOutput, CONTACT_CARD_EMBEDDABLE, } from '@kbn/embeddable-plugin/public/lib/test_samples'; import { @@ -287,7 +289,7 @@ test('searchSessionId is updated prior to child embeddable parent subscription e getSearchSessionIdFromURL: () => undefined, removeSessionIdFromUrl: () => {}, createSessionRestorationDataProvider: () => {}, - }, + } as unknown as DashboardCreationOptions['searchSessionSettings'], }); const embeddable = await dashboard.addNewEmbeddable< ContactCardEmbeddableInput, @@ -307,7 +309,7 @@ test('searchSessionId is updated prior to child embeddable parent subscription e }); const embeddableInput = embeddable.getInput(); - expect(embeddableInput.timeRange).toEqual({ + expect((embeddableInput as any).timeRange).toEqual({ to: 'now', from: 'now-7d', }); From bd6243c34d5088e0f3713ecced3dac853a960cae Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Mon, 24 Apr 2023 23:00:31 +0000 Subject: [PATCH 10/17] [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' --- .../state/diffing/dashboard_diffing_functions.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts index 4f00f982da14d..fe8e18528e2c0 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_functions.ts @@ -64,7 +64,9 @@ export const isKeyEqual = ( } if (diffingFunction?.prototype?.name === 'AsyncFunction') { - throw new Error(`The function for key "${key}" is async, must use isKeyEqualAsync for asynchronous functions`); + throw new Error( + `The function for key "${key}" is async, must use isKeyEqualAsync for asynchronous functions` + ); } return diffingFunction(propsAsNever); }; From f2afaa13ff81a0af0d104adbdeefcf73214bc972 Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 19:47:47 -0600 Subject: [PATCH 11/17] fix jest tests --- .../create/create_dashboard.test.ts | 2 +- .../dashboard_diffing_integration.test.ts | 28 +++++++++---------- .../diffing/dashboard_diffing_integration.ts | 6 ++-- .../public/embeddable/embeddable.test.tsx | 5 ---- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts index c5ec696c8ad0c..72bada198d613 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts @@ -313,5 +313,5 @@ test('searchSessionId is updated prior to child embeddable parent subscription e to: 'now', from: 'now-7d', }); - expect(embeddableInput.searchSessionId).toBe('searchSessionId2'); + expect(embeddableInput.searchSessionId).toBe(`searchSessionId${sessionCount}`); }); diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts index c0953f8bbc98a..ec53af6f6be0c 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts @@ -29,14 +29,14 @@ describe('getShouldRefresh', () => { ); describe('filter changes', () => { - test('should return false when filters do not change', async () => { + test('should return false when filters do not change', () => { const lastInput = { filters: [existsFilter], } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, lastInput)).toBe(false); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, lastInput)).toBe(false); }); - test('should return true when pinned filters change', async () => { + test('should return true when pinned filters change', () => { const pinnedFilter = pinFilter(existsFilter); const lastInput = { filters: [pinnedFilter], @@ -44,10 +44,10 @@ describe('getShouldRefresh', () => { const input = { filters: [toggleFilterNegated(pinnedFilter)], } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); }); - test('should return false when disabled filters change', async () => { + test('should return false when disabled filters change', () => { const disabledFilter = disableFilter(existsFilter); const lastInput = { filters: [disabledFilter], @@ -55,29 +55,29 @@ describe('getShouldRefresh', () => { const input = { filters: [toggleFilterNegated(disabledFilter)], } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(false); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(false); }); - test('should return false when pinned filter changes to unpinned', async () => { + test('should return false when pinned filter changes to unpinned', () => { const lastInput = { filters: [existsFilter], } as unknown as DashboardContainerInput; const input = { filters: [pinFilter(existsFilter)], } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(false); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(false); }); }); describe('timeRange changes', () => { - test('should return false when timeRange does not change', async () => { + test('should return false when timeRange does not change', () => { const lastInput = { timeRange: { from: 'now-15m', to: 'now' }, } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, lastInput)).toBe(false); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, lastInput)).toBe(false); }); - test('should return true when timeRange changes (timeRestore is true)', async () => { + test('should return true when timeRange changes (timeRestore is true)', () => { const lastInput = { timeRange: { from: 'now-15m', to: 'now' }, timeRestore: true, @@ -86,10 +86,10 @@ describe('getShouldRefresh', () => { timeRange: { from: 'now-30m', to: 'now' }, timeRestore: true, } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); }); - test('should return true when timeRange changes (timeRestore is false)', async () => { + test('should return true when timeRange changes (timeRestore is false)', () => { const lastInput = { timeRange: { from: 'now-15m', to: 'now' }, timeRestore: false, @@ -98,7 +98,7 @@ describe('getShouldRefresh', () => { timeRange: { from: 'now-30m', to: 'now' }, timeRestore: false, } as unknown as DashboardContainerInput; - expect(await getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); }); }); }); diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts index feb294ebbcd27..d4fd0917dd962 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts @@ -173,8 +173,10 @@ export function getShouldRefresh( input: DashboardContainerInput ): boolean { for (const key of sessionChangeKeys) { - if ( - !isKeyEqual( + if (input[key] === undefined && lastInput[key] === undefined) { + continue; + } + if (!isKeyEqual( key, { container: this, diff --git a/x-pack/plugins/lens/public/embeddable/embeddable.test.tsx b/x-pack/plugins/lens/public/embeddable/embeddable.test.tsx index c127e9f1130d3..5e48e1f46dd63 100644 --- a/x-pack/plugins/lens/public/embeddable/embeddable.test.tsx +++ b/x-pack/plugins/lens/public/embeddable/embeddable.test.tsx @@ -441,11 +441,6 @@ describe('embeddable', () => { expect(expressionRenderer).toHaveBeenCalledTimes(1); - embeddable.updateInput({ - filters: [{ meta: { alias: 'test', negate: false, disabled: false } }], - }); - await new Promise((resolve) => setTimeout(resolve, 0)); - embeddable.updateInput({ searchSessionId: 'nextSession', }); From 72fb6f10fed8d7dd71f8aaee3ae882ae7512175b Mon Sep 17 00:00:00 2001 From: nreese Date: Mon, 24 Apr 2023 19:51:15 -0600 Subject: [PATCH 12/17] add test case for key without custom diff function --- .../dashboard_diffing_integration.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts index ec53af6f6be0c..fcb62a1d0c7fc 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts @@ -101,4 +101,24 @@ describe('getShouldRefresh', () => { expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); }); }); + + describe('key without custom diffing function (syncColors)', () => { + test('should return false when syncColors do not change', () => { + const lastInput = { + syncColors: false, + } as unknown as DashboardContainerInput; + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, lastInput)).toBe(false); + }); + + test('should return true when syncColors change', () => { + const pinnedFilter = pinFilter(existsFilter); + const lastInput = { + syncColors: false, + } as unknown as DashboardContainerInput; + const input = { + syncColors: true, + } as unknown as DashboardContainerInput; + expect(getShouldRefresh.bind(dashboardContainerMock)(lastInput, input)).toBe(true); + }); + }); }); From 88daae1b786332f7f5781cea309453cda55c3dfe Mon Sep 17 00:00:00 2001 From: nreese Date: Tue, 25 Apr 2023 08:18:10 -0600 Subject: [PATCH 13/17] tslint --- .../embeddable/create/create_dashboard.test.ts | 1 + .../state/diffing/dashboard_diffing_integration.test.ts | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts index 72bada198d613..2983cc8d9d5f1 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts @@ -308,6 +308,7 @@ test('searchSessionId is updated prior to child embeddable parent subscription e }, }); + expect(sessionCount).toBeGreaterThan(1); const embeddableInput = embeddable.getInput(); expect((embeddableInput as any).timeRange).toEqual({ to: 'now', diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts index fcb62a1d0c7fc..b79eb27af3d79 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.test.ts @@ -111,7 +111,6 @@ describe('getShouldRefresh', () => { }); test('should return true when syncColors change', () => { - const pinnedFilter = pinFilter(existsFilter); const lastInput = { syncColors: false, } as unknown as DashboardContainerInput; From 91e8a935bb8ac577526b916c7da35e3df5867d56 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Tue, 25 Apr 2023 14:56:23 +0000 Subject: [PATCH 14/17] [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' --- .../state/diffing/dashboard_diffing_integration.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts index d4fd0917dd962..b1ee66fd02c75 100644 --- a/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts +++ b/src/plugins/dashboard/public/dashboard_container/state/diffing/dashboard_diffing_integration.ts @@ -176,7 +176,8 @@ export function getShouldRefresh( if (input[key] === undefined && lastInput[key] === undefined) { continue; } - if (!isKeyEqual( + if ( + !isKeyEqual( key, { container: this, From 878c9443da1b0c6232368d189fcc0382429288da Mon Sep 17 00:00:00 2001 From: nreese Date: Tue, 25 Apr 2023 12:09:20 -0600 Subject: [PATCH 15/17] fix panel_time_range test --- .../functional/apps/dashboard/group2/panel_time_range.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/test/functional/apps/dashboard/group2/panel_time_range.ts b/x-pack/test/functional/apps/dashboard/group2/panel_time_range.ts index ee07446783603..2295c90d60c65 100644 --- a/x-pack/test/functional/apps/dashboard/group2/panel_time_range.ts +++ b/x-pack/test/functional/apps/dashboard/group2/panel_time_range.ts @@ -50,7 +50,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await dashboardCustomizePanel.clickSaveButton(); await PageObjects.dashboard.waitForRenderComplete(); await dashboardBadgeActions.expectExistsTimeRangeBadgeAction(); - expect(await testSubjects.exists('emptyPlaceholder')); + expect(await testSubjects.exists('emptyPlaceholder')).to.be(true); await PageObjects.dashboard.clickQuickSave(); }); @@ -60,7 +60,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await dashboardCustomizePanel.clickSaveButton(); await PageObjects.dashboard.waitForRenderComplete(); await dashboardBadgeActions.expectMissingTimeRangeBadgeAction(); - expect(await testSubjects.exists('xyVisChart')); + expect(await testSubjects.exists('xyVisChart')).to.be(true); }); }); @@ -74,7 +74,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await dashboardCustomizePanel.clickSaveButton(); await PageObjects.dashboard.waitForRenderComplete(); await dashboardBadgeActions.expectExistsTimeRangeBadgeAction(); - expect(await testSubjects.exists('emptyPlaceholder')); + expect(await testSubjects.exists('emptyPlaceholder')).to.be(true); await PageObjects.dashboard.clickQuickSave(); }); @@ -84,7 +84,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await dashboardCustomizePanel.clickSaveButton(); await PageObjects.dashboard.waitForRenderComplete(); await dashboardBadgeActions.expectMissingTimeRangeBadgeAction(); - expect(await testSubjects.exists('xyVisChart')); + expect(await testSubjects.exists('xyVisChart')).to.be(true); }); }); From 976fd40505e2c593461e69c08abe710eae3a9251 Mon Sep 17 00:00:00 2001 From: nreese Date: Tue, 25 Apr 2023 14:48:16 -0600 Subject: [PATCH 16/17] fix typo in comment --- .../embeddable/create/create_dashboard.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts index 2983cc8d9d5f1..947b062b3a551 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts @@ -262,7 +262,7 @@ test('creates a control group from the control group factory and waits for it to * 1) dashboard instance searchSessionId state * 2) child input on parent input changes * - * Rxjs subscriptions are executioned in the order that they are created. + * Rxjs subscriptions are executed in the order that they are created. * This test ensures that searchSessionId update subscription is created before child input subscription * to ensure child input subscription includes updated searchSessionId. */ From 5b6365ddb3d040e26be48bcac19db7f532b6643d Mon Sep 17 00:00:00 2001 From: nreese Date: Tue, 25 Apr 2023 15:40:35 -0600 Subject: [PATCH 17/17] fix session test --- test/plugin_functional/test_suites/data_plugin/session.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/plugin_functional/test_suites/data_plugin/session.ts b/test/plugin_functional/test_suites/data_plugin/session.ts index 6c485a76db32e..469e6f992e79f 100644 --- a/test/plugin_functional/test_suites/data_plugin/session.ts +++ b/test/plugin_functional/test_suites/data_plugin/session.ts @@ -106,7 +106,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide }); it('starts a session on filter change', async () => { - await filterBar.removeAllFilters(); + await filterBar.removeFilter('animal'); const sessionIds = await getSessionIds(); expect(sessionIds.length).to.be(1); });