From 96d77a6e1b94dbbbc580ac5de2f22ee07cacb5ba Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 10:34:12 -0600 Subject: [PATCH 01/10] passing working state to workspace panel wrapper --- .../workspace_panel/workspace_panel.tsx | 156 +++++++----------- 1 file changed, 58 insertions(+), 98 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index c98b40c275b03..d21a72d257a62 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -128,7 +128,7 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac * This function returns the appropriate arguments depending on whether * or not appliedState is passed in */ -const getBuildExpressionArgs = ({ +const getWorkspaceArgs = ({ appliedState, visualization, datasourceStates, @@ -223,7 +223,7 @@ const checkUnknownVis = ( return visualization.activeId && !activeVisualization; }; -const generateExpression = ({ +const generateWorkspaceExpression = ({ errors: { hasConfigurationValidationError, hasMissingRefsErrors, hasUnknownVisError }, activeVisualization, visualization, @@ -287,53 +287,24 @@ const generateExpression = ({ * Since state is applied when the user saves the visualization, the save button * should be disabled when the visualization configuration that would actually be saved is invalid */ -const areUnappliedChangesSaveable = ({ - _visualization, - _datasourceStates, - _activeDatasourceId, - visualizationMap, - datasourceMap, - framePublicAPI, -}: { - _visualization: VisualizationState; - _datasourceStates: DatasourceStates; - _activeDatasourceId: string | null; - visualizationMap: VisualizationMap; +const areChangesSaveable = (config: { + visualization: VisualizationState; + datasourceStates: DatasourceStates; + activeDatasourceId: string | null; + activeVisualization: Visualization | null; datasourceMap: DatasourceMap; - framePublicAPI: FramePublicAPI; + datasourceLayers: FramePublicAPI['datasourceLayers']; }) => { - const unappliedArgs = getBuildExpressionArgs({ - appliedState: undefined, // get the UNAPPLIED state args - visualization: _visualization, - datasourceStates: _datasourceStates, - activeDatasourceId: _activeDatasourceId, - framePublicAPI, - visualizationMap, - }); - - const { activeDatasourceId, datasourceStates, visualization, activeVisualization } = - unappliedArgs; - - const configurationValidationError = checkConfiguration({ - activeVisualization, - visualization, - activeDatasourceId, - datasourceStates, - datasourceMap, - datasourceLayers: framePublicAPI.datasourceLayers, - }); + const configurationValidationError = checkConfiguration(config); const missingRefsErrors = checkMissingRefs({ - activeDatasourceId, - datasourceStates, - datasourceMap, + ...config, }); - const unknownVisError = checkUnknownVis(visualization, activeVisualization); + const unknownVisError = checkUnknownVis(config.visualization, config.activeVisualization); - const { expression: workingExpression } = generateExpression({ - ...unappliedArgs, - datasourceMap, + const { expression: workingExpression } = generateWorkspaceExpression({ + ...config, errors: { hasConfigurationValidationError: Boolean(configurationValidationError?.length), hasMissingRefsErrors: Boolean(missingRefsErrors.length), @@ -358,25 +329,27 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const dispatchLens = useLensDispatch(); const isFullscreen = useLensSelector(selectIsFullscreenDatasource); - // The following variables shouldn't be used for logic in this component - // Use the non-underscore versions instead, because they will reflect the applied state - // even when the user has unapplied changes + // The following underscore variables shouldn't be used for logic in this component + // Use appliedArgs or unappliedArgs instead const _visualization = useLensSelector(selectVisualization); const _datasourceStates = useLensSelector(selectDatasourceStates); const _activeDatasourceId = useLensSelector(selectActiveDatasourceId); const _appliedState = useLensSelector(selectAppliedState); - const { - visualization, - datasourceStates, - activeDatasourceId, - datasourceLayers, - activeVisualization, - } = getBuildExpressionArgs({ + const appliedArgs = getWorkspaceArgs({ + appliedState: _appliedState, + visualization: _visualization, + datasourceStates: _datasourceStates, + activeDatasourceId: _activeDatasourceId, + visualizationMap, + framePublicAPI, + }); + + const unappliedArgs = getWorkspaceArgs({ + appliedState: undefined, visualization: _visualization, datasourceStates: _datasourceStates, activeDatasourceId: _activeDatasourceId, - appliedState: _appliedState, visualizationMap, framePublicAPI, }); @@ -392,36 +365,39 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const configurationValidationError = useMemo( () => checkConfiguration({ - activeVisualization, - visualization, - activeDatasourceId, - datasourceStates, + ...appliedArgs, datasourceMap, - datasourceLayers, }), // eslint-disable-next-line react-hooks/exhaustive-deps - [activeVisualization, visualization.state, activeDatasourceId, datasourceMap, datasourceStates] + [ + appliedArgs.activeVisualization, + appliedArgs.visualization.state, + appliedArgs.activeDatasourceId, + datasourceMap, + appliedArgs.datasourceStates, + ] ); const missingRefsErrors = checkMissingRefs({ - activeDatasourceId, + activeDatasourceId: appliedArgs.activeDatasourceId, + datasourceStates: appliedArgs.datasourceStates, datasourceMap, - datasourceStates, }); - const unknownVisError = checkUnknownVis(visualization, activeVisualization); + + const unknownVisError = checkUnknownVis( + appliedArgs.visualization, + appliedArgs.activeVisualization + ); const expression = useMemo(() => { - const { expression: _expression, expressionBuildError } = generateExpression({ + const { expression: _expression, expressionBuildError } = generateWorkspaceExpression({ errors: { hasConfigurationValidationError: Boolean(configurationValidationError?.length), hasMissingRefsErrors: Boolean(missingRefsErrors?.length), hasUnknownVisError: Boolean(unknownVisError), }, - activeVisualization, - visualization, + ...appliedArgs, datasourceMap, - datasourceStates, - datasourceLayers, }); if (expressionBuildError) { @@ -433,11 +409,8 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ configurationValidationError?.length, missingRefsErrors?.length, unknownVisError, - activeVisualization, - visualization, + appliedArgs, datasourceMap, - datasourceStates, - datasourceLayers, ]); const expressionExists = Boolean(expression); @@ -448,25 +421,12 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ if (changesApplied) { return expressionExists; } else { - return areUnappliedChangesSaveable({ - _visualization, - _datasourceStates, - _activeDatasourceId, - visualizationMap, + return areChangesSaveable({ + ...unappliedArgs, datasourceMap, - framePublicAPI, }); } - }, [ - _activeDatasourceId, - _datasourceStates, - _visualization, - changesApplied, - datasourceMap, - expressionExists, - framePublicAPI, - visualizationMap, - ]); + }, [changesApplied, datasourceMap, expressionExists, unappliedArgs]); useEffect(() => { dispatchLens(setSaveable(isSaveable)); @@ -494,16 +454,16 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }, }); } - if (isLensEditEvent(event) && activeVisualization?.onEditAction) { + if (isLensEditEvent(event) && appliedArgs.activeVisualization?.onEditAction) { dispatchLens( editVisualizationAction({ - visualizationId: activeVisualization.id, + visualizationId: appliedArgs.activeVisualization.id, event, }) ); } }, - [plugins.uiActions, activeVisualization, dispatchLens] + [plugins.uiActions, appliedArgs.activeVisualization, dispatchLens] ); useEffect(() => { @@ -585,7 +545,7 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ localState={{ ...localState, configurationValidationError, missingRefsErrors }} ExpressionRendererComponent={ExpressionRendererComponent} application={core.application} - activeDatasourceId={activeDatasourceId} + activeDatasourceId={appliedArgs.activeDatasourceId} /> ); }; @@ -594,11 +554,11 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const renderDragDrop = () => { const customWorkspaceRenderer = - activeDatasourceId && - datasourceMap[activeDatasourceId]?.getCustomWorkspaceRenderer && + appliedArgs.activeDatasourceId && + datasourceMap[appliedArgs.activeDatasourceId]?.getCustomWorkspaceRenderer && dragDropContext.dragging - ? datasourceMap[activeDatasourceId].getCustomWorkspaceRenderer!( - datasourceStates[activeDatasourceId].state, + ? datasourceMap[appliedArgs.activeDatasourceId].getCustomWorkspaceRenderer!( + appliedArgs.datasourceStates[appliedArgs.activeDatasourceId].state, dragDropContext.dragging ) : undefined; @@ -628,9 +588,9 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ return ( Date: Thu, 24 Feb 2022 13:12:57 -0600 Subject: [PATCH 02/10] caching expression strategy --- .../workspace_panel/workspace_panel.tsx | 401 ++++++------------ .../public/state_management/lens_slice.ts | 25 +- .../lens/public/state_management/selectors.ts | 36 +- .../lens/public/state_management/types.ts | 9 +- 4 files changed, 141 insertions(+), 330 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index d21a72d257a62..550db83b2999a 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -38,7 +38,6 @@ import { DatasourceMap, DatasourceFixAction, Suggestion, - Visualization, } from '../../../types'; import { DragDrop, DragContext, DragDropIdentifier } from '../../../drag_drop'; import { switchToSuggestion } from '../suggestion_helpers'; @@ -67,14 +66,13 @@ import { selectDatasourceStates, selectActiveDatasourceId, selectSearchSessionId, - selectAppliedState, - VisualizationState, - DatasourceStates, - AppliedState, - selectChangesApplied, + selectAutoApplyEnabled, + selectApplyChangesCounter, + selectTriggerApplyChanges, } from '../../../state_management'; import type { LensInspector } from '../../../lens_inspector_service'; import { inferTimeField } from '../../../utils'; +import { setChangesApplied } from '../../../state_management/lens_slice'; export interface WorkspacePanelProps { visualizationMap: VisualizationMap; @@ -124,74 +122,47 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac ); }); -/** - * This function returns the appropriate arguments depending on whether - * or not appliedState is passed in - */ -const getWorkspaceArgs = ({ - appliedState, - visualization, - datasourceStates, - activeDatasourceId, - framePublicAPI: { datasourceLayers, appliedDatasourceLayers }, +let finishedInitialRender: boolean; + +// Exported for testing purposes only. +export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ + framePublicAPI, visualizationMap, -}: { - appliedState?: AppliedState; - visualization: VisualizationState; - datasourceStates: DatasourceStates; - activeDatasourceId: string | null; - framePublicAPI: FramePublicAPI; - visualizationMap: VisualizationMap; -}) => { - const visualizationState = appliedState?.visualization || visualization; - return { - visualization: visualizationState, - datasourceStates: appliedState?.datasourceStates || datasourceStates, - activeDatasourceId: appliedState?.activeDatasourceId || activeDatasourceId, - datasourceLayers: appliedDatasourceLayers || datasourceLayers, - activeVisualization: visualization.activeId - ? visualizationMap[visualizationState.activeId as string] - : null, - }; -}; + datasourceMap, + core, + plugins, + ExpressionRenderer: ExpressionRendererComponent, + suggestionForDraggedField, + lensInspector, +}: Omit & { + suggestionForDraggedField: Suggestion | undefined; +}) { + const dispatchLens = useLensDispatch(); + const isFullscreen = useLensSelector(selectIsFullscreenDatasource); + const visualization = useLensSelector(selectVisualization); + const activeDatasourceId = useLensSelector(selectActiveDatasourceId); + const datasourceStates = useLensSelector(selectDatasourceStates); + const autoApplyEnabled = useLensSelector(selectAutoApplyEnabled); + const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender; -interface ErrorDescription { - shortMessage: string; - longMessage: React.ReactNode; -} + const [expressionToRender, setExpressionToRender] = useState(); -const checkConfiguration = ({ - activeVisualization, - visualization, - activeDatasourceId, - datasourceStates, - datasourceMap, - datasourceLayers, -}: { - activeVisualization: Visualization | null; - visualization: VisualizationState; - activeDatasourceId: string | null; - datasourceMap: DatasourceMap; - datasourceStates: DatasourceStates; - datasourceLayers: FramePublicAPI['datasourceLayers']; -}) => - validateDatasourceAndVisualization( - activeDatasourceId ? datasourceMap[activeDatasourceId] : null, - activeDatasourceId && datasourceStates[activeDatasourceId]?.state, - activeVisualization, - visualization.state, - { datasourceLayers } - ); + useEffect(() => { + return () => { + finishedInitialRender = false; + }; + }, []); + + const { datasourceLayers } = framePublicAPI; + const [localState, setLocalState] = useState({ + expressionBuildError: undefined, + expandError: false, + }); + + const activeVisualization = visualization.activeId + ? visualizationMap[visualization.activeId] + : null; -const checkMissingRefs = ({ - activeDatasourceId, - datasourceMap, - datasourceStates, -}: { - activeDatasourceId: string | null; - datasourceMap: DatasourceMap; - datasourceStates: DatasourceStates; -}) => { const missingIndexPatterns = getMissingIndexPattern( activeDatasourceId ? datasourceMap[activeDatasourceId] : null, activeDatasourceId ? datasourceStates[activeDatasourceId] : null @@ -213,224 +184,94 @@ const checkMissingRefs = ({ ] : []; - return missingRefsErrors; -}; - -const checkUnknownVis = ( - visualization: VisualizationState, - activeVisualization: Visualization | null -) => { - return visualization.activeId && !activeVisualization; -}; - -const generateWorkspaceExpression = ({ - errors: { hasConfigurationValidationError, hasMissingRefsErrors, hasUnknownVisError }, - activeVisualization, - visualization, - datasourceMap, - datasourceStates, - datasourceLayers, -}: { - errors: { - hasConfigurationValidationError: boolean; - hasMissingRefsErrors: boolean; - hasUnknownVisError: boolean; - }; - activeVisualization: Visualization | null; - visualization: VisualizationState; - datasourceMap: DatasourceMap; - datasourceStates: DatasourceStates; - datasourceLayers: FramePublicAPI['datasourceLayers']; -}) => { - let expression; - let expressionBuildError: ErrorDescription[] | undefined; - if (!hasConfigurationValidationError && !hasMissingRefsErrors && !hasUnknownVisError) { - try { - const ast = buildExpression({ - visualization: activeVisualization, - visualizationState: visualization.state, - datasourceMap, - datasourceStates, - datasourceLayers, - }); - - if (ast) { - // expression has to be turned into a string for dirty checking - if the ast is rebuilt, - // turning it into a string will make sure the expression renderer only re-renders if the - // expression actually changed. - expression = toExpression(ast); - } else { - expression = null; - } - } catch (e) { - const buildMessages = activeVisualization?.getErrorMessages(visualization.state); - const defaultMessage = { - shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', { - defaultMessage: 'An unexpected error occurred while preparing the chart', - }), - longMessage: e.toString(), - }; - // Most likely an error in the expression provided by a datasource or visualization - expressionBuildError = buildMessages ?? [defaultMessage]; - } - } - if (hasUnknownVisError) { - expressionBuildError = [getUnknownVisualizationTypeError(visualization.activeId!)]; - } - - return { expression, expressionBuildError }; -}; - -/** - * Checks whether the user's unapplied changes are saveable (can generate a valid expression) - * - * Since state is applied when the user saves the visualization, the save button - * should be disabled when the visualization configuration that would actually be saved is invalid - */ -const areChangesSaveable = (config: { - visualization: VisualizationState; - datasourceStates: DatasourceStates; - activeDatasourceId: string | null; - activeVisualization: Visualization | null; - datasourceMap: DatasourceMap; - datasourceLayers: FramePublicAPI['datasourceLayers']; -}) => { - const configurationValidationError = checkConfiguration(config); - - const missingRefsErrors = checkMissingRefs({ - ...config, - }); - - const unknownVisError = checkUnknownVis(config.visualization, config.activeVisualization); - - const { expression: workingExpression } = generateWorkspaceExpression({ - ...config, - errors: { - hasConfigurationValidationError: Boolean(configurationValidationError?.length), - hasMissingRefsErrors: Boolean(missingRefsErrors.length), - hasUnknownVisError: Boolean(unknownVisError), - }, - }); - return Boolean(workingExpression); -}; - -const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ - framePublicAPI, - visualizationMap, - datasourceMap, - core, - plugins, - ExpressionRenderer: ExpressionRendererComponent, - suggestionForDraggedField, - lensInspector, -}: Omit & { - suggestionForDraggedField: Suggestion | undefined; -}) { - const dispatchLens = useLensDispatch(); - const isFullscreen = useLensSelector(selectIsFullscreenDatasource); - - // The following underscore variables shouldn't be used for logic in this component - // Use appliedArgs or unappliedArgs instead - const _visualization = useLensSelector(selectVisualization); - const _datasourceStates = useLensSelector(selectDatasourceStates); - const _activeDatasourceId = useLensSelector(selectActiveDatasourceId); - const _appliedState = useLensSelector(selectAppliedState); - - const appliedArgs = getWorkspaceArgs({ - appliedState: _appliedState, - visualization: _visualization, - datasourceStates: _datasourceStates, - activeDatasourceId: _activeDatasourceId, - visualizationMap, - framePublicAPI, - }); - - const unappliedArgs = getWorkspaceArgs({ - appliedState: undefined, - visualization: _visualization, - datasourceStates: _datasourceStates, - activeDatasourceId: _activeDatasourceId, - visualizationMap, - framePublicAPI, - }); - - const [localState, setLocalState] = useState({ - expressionBuildError: undefined, - expandError: false, - }); + const unknownVisError = visualization.activeId && !activeVisualization; // Note: mind to all these eslint disable lines: the frameAPI will change too frequently // and to prevent race conditions it is ok to leave them there. const configurationValidationError = useMemo( () => - checkConfiguration({ - ...appliedArgs, - datasourceMap, - }), + validateDatasourceAndVisualization( + activeDatasourceId ? datasourceMap[activeDatasourceId] : null, + activeDatasourceId && datasourceStates[activeDatasourceId]?.state, + activeVisualization, + visualization.state, + framePublicAPI + ), // eslint-disable-next-line react-hooks/exhaustive-deps - [ - appliedArgs.activeVisualization, - appliedArgs.visualization.state, - appliedArgs.activeDatasourceId, - datasourceMap, - appliedArgs.datasourceStates, - ] - ); - - const missingRefsErrors = checkMissingRefs({ - activeDatasourceId: appliedArgs.activeDatasourceId, - datasourceStates: appliedArgs.datasourceStates, - datasourceMap, - }); - - const unknownVisError = checkUnknownVis( - appliedArgs.visualization, - appliedArgs.activeVisualization + [activeVisualization, visualization.state, activeDatasourceId, datasourceMap, datasourceStates] ); const expression = useMemo(() => { - const { expression: _expression, expressionBuildError } = generateWorkspaceExpression({ - errors: { - hasConfigurationValidationError: Boolean(configurationValidationError?.length), - hasMissingRefsErrors: Boolean(missingRefsErrors?.length), - hasUnknownVisError: Boolean(unknownVisError), - }, - ...appliedArgs, - datasourceMap, - }); + if (!configurationValidationError?.length && !missingRefsErrors.length && !unknownVisError) { + try { + const ast = buildExpression({ + visualization: activeVisualization, + visualizationState: visualization.state, + datasourceMap, + datasourceStates, + datasourceLayers, + }); - if (expressionBuildError) { - setLocalState((state) => ({ ...state, expressionBuildError })); + if (ast) { + // expression has to be turned into a string for dirty checking - if the ast is rebuilt, + // turning it into a string will make sure the expression renderer only re-renders if the + // expression actually changed. + return toExpression(ast); + } else { + return null; + } + } catch (e) { + const buildMessages = activeVisualization?.getErrorMessages(visualization.state); + const defaultMessage = { + shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', { + defaultMessage: 'An unexpected error occurred while preparing the chart', + }), + longMessage: e.toString(), + }; + // Most likely an error in the expression provided by a datasource or visualization + setLocalState((s) => ({ + ...s, + expressionBuildError: buildMessages ?? [defaultMessage], + })); + } + } + if (unknownVisError) { + setLocalState((s) => ({ + ...s, + expressionBuildError: [getUnknownVisualizationTypeError(visualization.activeId!)], + })); } - - return _expression; }, [ + activeVisualization, + visualization.state, + datasourceMap, + datasourceStates, + datasourceLayers, configurationValidationError?.length, - missingRefsErrors?.length, + missingRefsErrors.length, unknownVisError, - appliedArgs, - datasourceMap, + visualization.activeId, ]); - const expressionExists = Boolean(expression); + useEffect(() => { + if (shouldApplyChanges) { + setExpressionToRender(expression); + } + }, [expression, shouldApplyChanges]); - const changesApplied = useLensSelector(selectChangesApplied); + useEffect(() => { + dispatchLens(setChangesApplied(expression === expressionToRender)); + }, [expression, dispatchLens, expressionToRender]); - const isSaveable = useMemo(() => { - if (changesApplied) { - return expressionExists; - } else { - return areChangesSaveable({ - ...unappliedArgs, - datasourceMap, - }); - } - }, [changesApplied, datasourceMap, expressionExists, unappliedArgs]); + const expressionExists = Boolean(expressionToRender); + if (expressionExists) { + finishedInitialRender = true; + } useEffect(() => { - dispatchLens(setSaveable(isSaveable)); - }, [isSaveable, dispatchLens]); + dispatchLens(setSaveable(expressionExists)); + }, [expressionExists, dispatchLens]); const onEvent = useCallback( (event: ExpressionRendererEvent) => { @@ -454,16 +295,16 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }, }); } - if (isLensEditEvent(event) && appliedArgs.activeVisualization?.onEditAction) { + if (isLensEditEvent(event) && activeVisualization?.onEditAction) { dispatchLens( editVisualizationAction({ - visualizationId: appliedArgs.activeVisualization.id, + visualizationId: activeVisualization.id, event, }) ); } }, - [plugins.uiActions, appliedArgs.activeVisualization, dispatchLens] + [plugins.uiActions, activeVisualization, dispatchLens] ); useEffect(() => { @@ -532,12 +373,12 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }; const renderVisualization = () => { - if (expression === null) { + if (!expressionExists) { return renderEmptyWorkspace(); } return ( ); }; @@ -554,11 +395,11 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const renderDragDrop = () => { const customWorkspaceRenderer = - appliedArgs.activeDatasourceId && - datasourceMap[appliedArgs.activeDatasourceId]?.getCustomWorkspaceRenderer && + activeDatasourceId && + datasourceMap[activeDatasourceId]?.getCustomWorkspaceRenderer && dragDropContext.dragging - ? datasourceMap[appliedArgs.activeDatasourceId].getCustomWorkspaceRenderer!( - appliedArgs.datasourceStates[appliedArgs.activeDatasourceId].state, + ? datasourceMap[activeDatasourceId].getCustomWorkspaceRenderer!( + datasourceStates[activeDatasourceId].state, dragDropContext.dragging ) : undefined; @@ -588,9 +429,9 @@ const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ return ( ('lens/onAc export const setSaveable = createAction('lens/setSaveable'); export const enableAutoApply = createAction('lens/enableAutoApply'); export const disableAutoApply = createAction('lens/disableAutoApply'); -// this action is only relevant when auto-apply is disabled export const applyChanges = createAction('lens/applyChanges'); +export const setChangesApplied = createAction('lens/setChangesApplied'); export const updateState = createAction<{ updater: (prevState: LensAppState) => LensAppState; }>('lens/updateState'); @@ -169,6 +169,7 @@ export const lensActions = { enableAutoApply, disableAutoApply, applyChanges, + setChangesApplied, updateState, updateDatasourceState, updateVisualizationState, @@ -188,14 +189,6 @@ export const lensActions = { setLayerDefaultDimension, }; -const buildAppliedState = (state: LensAppState) => { - return { - activeDatasourceId: state.activeDatasourceId, - visualization: cloneDeep(state.visualization), - datasourceStates: cloneDeep(state.datasourceStates), - }; -}; - export const makeLensReducer = (storeDeps: LensStoreDeps) => { const { datasourceMap, visualizationMap } = storeDeps; return createReducer(initialState, { @@ -218,15 +211,19 @@ export const makeLensReducer = (storeDeps: LensStoreDeps) => { }; }, [enableAutoApply.type]: (state) => { - delete state.appliedState; + state.applyChangesDisabled = false; }, [disableAutoApply.type]: (state) => { - state.appliedState = buildAppliedState(state); + state.applyChangesDisabled = true; }, [applyChanges.type]: (state) => { - if (state.appliedState) { - state.appliedState = buildAppliedState(state); + if (typeof state.applyChangesCounter === 'undefined') { + state.applyChangesCounter = 0; } + state.applyChangesCounter!++; + }, + [setChangesApplied.type]: (state, { payload: applied }) => { + state.changesApplied = applied; }, [updateState.type]: ( state, diff --git a/x-pack/plugins/lens/public/state_management/selectors.ts b/x-pack/plugins/lens/public/state_management/selectors.ts index a183197109739..4fca9a8d258c3 100644 --- a/x-pack/plugins/lens/public/state_management/selectors.ts +++ b/x-pack/plugins/lens/public/state_management/selectors.ts @@ -8,9 +8,8 @@ import { createSelector } from '@reduxjs/toolkit'; import { SavedObjectReference } from 'kibana/server'; import { FilterManager } from 'src/plugins/data/public'; -import { isEqual } from 'lodash'; import { LensState } from './types'; -import { Datasource, DatasourceMap, FramePublicAPI, VisualizationMap } from '../types'; +import { Datasource, DatasourceMap, VisualizationMap } from '../types'; import { getDatasourceLayers } from '../editor_frame_service/editor_frame'; export const selectPersistedDoc = (state: LensState) => state.lens.persistedDoc; @@ -20,28 +19,15 @@ export const selectFilters = (state: LensState) => state.lens.filters; export const selectResolvedDateRange = (state: LensState) => state.lens.resolvedDateRange; export const selectVisualization = (state: LensState) => state.lens.visualization; export const selectStagedPreview = (state: LensState) => state.lens.stagedPreview; -export const selectAppliedState = (state: LensState) => state.lens.appliedState; +export const selectAutoApplyEnabled = (state: LensState) => !state.lens.applyChangesDisabled; +export const selectChangesApplied = (state: LensState) => Boolean(state.lens.changesApplied); +export const selectApplyChangesCounter = (state: LensState) => state.lens.applyChangesCounter; export const selectDatasourceStates = (state: LensState) => state.lens.datasourceStates; export const selectActiveDatasourceId = (state: LensState) => state.lens.activeDatasourceId; export const selectActiveData = (state: LensState) => state.lens.activeData; export const selectIsFullscreenDatasource = (state: LensState) => Boolean(state.lens.isFullscreenDatasource); -export const selectChangesApplied = createSelector( - [selectAppliedState, selectVisualization, selectDatasourceStates, selectActiveDatasourceId], - (appliedState, visualization, datasourceStates, activeDatasourceId) => { - if (!appliedState) return true; // auto-apply is enabled - - const workingState = { activeDatasourceId, visualization, datasourceStates }; - return isEqual(appliedState, workingState); - } -); - -export const selectAutoApplyEnabled = createSelector( - [selectAppliedState], - (appliedState) => !Boolean(appliedState) -); - export const selectExecutionContext = createSelector( [selectQuery, selectFilters, selectResolvedDateRange], (query, filters, dateRange) => ({ @@ -167,23 +153,13 @@ export const selectDatasourceLayers = createSelector( export const selectFramePublicAPI = createSelector( [ selectDatasourceStates, - selectAppliedState, selectActiveData, selectInjectedDependencies as SelectInjectedDependenciesFunction, ], - (datasourceStates, appliedState, activeData, datasourceMap) => { - const api: FramePublicAPI = { + (datasourceStates, activeData, datasourceMap) => { + return { datasourceLayers: getDatasourceLayers(datasourceStates, datasourceMap), activeData, }; - - if (appliedState) { - api.appliedDatasourceLayers = getDatasourceLayers( - appliedState.datasourceStates, - datasourceMap - ); - } - - return api; } ); diff --git a/x-pack/plugins/lens/public/state_management/types.ts b/x-pack/plugins/lens/public/state_management/types.ts index 38cc580cf525d..256100a2287ff 100644 --- a/x-pack/plugins/lens/public/state_management/types.ts +++ b/x-pack/plugins/lens/public/state_management/types.ts @@ -30,15 +30,12 @@ export interface PreviewState { visualization: VisualizationState; datasourceStates: DatasourceStates; } -export interface AppliedState { - activeDatasourceId: EditorFrameState['activeDatasourceId']; - visualization: VisualizationState; - datasourceStates: DatasourceStates; -} export interface EditorFrameState extends PreviewState { activeDatasourceId: string | null; stagedPreview?: PreviewState; - appliedState?: AppliedState; + applyChangesDisabled?: boolean; + applyChangesCounter?: number; + changesApplied?: boolean; isFullscreenDatasource?: boolean; } export interface LensAppState extends EditorFrameState { From 59d1eceb0f081d2a9852a9da5b0894f0ee5875fe Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 14:03:10 -0600 Subject: [PATCH 03/10] apply-changes counter --- .../editor_frame/workspace_panel/workspace_panel.tsx | 8 +++++++- x-pack/plugins/lens/public/state_management/selectors.ts | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index 550db83b2999a..d74d8d090f05e 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -123,6 +123,7 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac }); let finishedInitialRender: boolean; +let lastApplyChangesCounter = 0; // Exported for testing purposes only. export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ @@ -143,7 +144,12 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const activeDatasourceId = useLensSelector(selectActiveDatasourceId); const datasourceStates = useLensSelector(selectDatasourceStates); const autoApplyEnabled = useLensSelector(selectAutoApplyEnabled); - const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender; + const applyChangesCounter = useLensSelector(selectApplyChangesCounter); + + const triggerApply = applyChangesCounter !== lastApplyChangesCounter; + lastApplyChangesCounter = applyChangesCounter; + + const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender || triggerApply; const [expressionToRender, setExpressionToRender] = useState(); diff --git a/x-pack/plugins/lens/public/state_management/selectors.ts b/x-pack/plugins/lens/public/state_management/selectors.ts index 4fca9a8d258c3..365e05a282014 100644 --- a/x-pack/plugins/lens/public/state_management/selectors.ts +++ b/x-pack/plugins/lens/public/state_management/selectors.ts @@ -21,7 +21,7 @@ export const selectVisualization = (state: LensState) => state.lens.visualizatio export const selectStagedPreview = (state: LensState) => state.lens.stagedPreview; export const selectAutoApplyEnabled = (state: LensState) => !state.lens.applyChangesDisabled; export const selectChangesApplied = (state: LensState) => Boolean(state.lens.changesApplied); -export const selectApplyChangesCounter = (state: LensState) => state.lens.applyChangesCounter; +export const selectApplyChangesCounter = (state: LensState) => state.lens.applyChangesCounter || 0; export const selectDatasourceStates = (state: LensState) => state.lens.datasourceStates; export const selectActiveDatasourceId = (state: LensState) => state.lens.activeDatasourceId; export const selectActiveData = (state: LensState) => state.lens.activeData; From ff5465e37cd9c9dde60d5702ab715c343da6365a Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 14:05:43 -0600 Subject: [PATCH 04/10] move initial render flag to local state --- .../workspace_panel/workspace_panel.tsx | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index d74d8d090f05e..a99585cd8f4a2 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -122,7 +122,6 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac ); }); -let finishedInitialRender: boolean; let lastApplyChangesCounter = 0; // Exported for testing purposes only. @@ -149,15 +148,10 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const triggerApply = applyChangesCounter !== lastApplyChangesCounter; lastApplyChangesCounter = applyChangesCounter; - const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender || triggerApply; - const [expressionToRender, setExpressionToRender] = useState(); + const [finishedInitialRender, setFinishedInitialRender] = useState(false); - useEffect(() => { - return () => { - finishedInitialRender = false; - }; - }, []); + const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender || triggerApply; const { datasourceLayers } = framePublicAPI; const [localState, setLocalState] = useState({ @@ -271,8 +265,8 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }, [expression, dispatchLens, expressionToRender]); const expressionExists = Boolean(expressionToRender); - if (expressionExists) { - finishedInitialRender = true; + if (expressionExists && !finishedInitialRender) { + setFinishedInitialRender(true); } useEffect(() => { From 638fd5c9a5fc9718c5d8828526841db1ee9f66ed Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 15:08:33 -0600 Subject: [PATCH 05/10] fix expression check --- .../workspace_panel/workspace_panel.test.tsx | 86 +++++++++---------- .../workspace_panel/workspace_panel.tsx | 14 +-- 2 files changed, 48 insertions(+), 52 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index b62ea4271ce1c..d7f45930d7676 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -177,6 +177,8 @@ describe('workspace_panel', () => { instance = mounted.instance; + instance.update(); + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` "kibana | lens_merge_tables layerIds=\\"first\\" tables={datasource} @@ -184,26 +186,14 @@ describe('workspace_panel', () => { `); }); - it('should use applied state when available (when auto-apply is disabled)', async () => { + it.skip('should give user control when auto-apply disabled', async () => { const framePublicAPI = createMockFramePublicAPI(); framePublicAPI.datasourceLayers = { first: mockDatasource.publicAPIMock, }; - framePublicAPI.appliedDatasourceLayers = { - first: { ...mockDatasource.publicAPIMock, applied: true } as DatasourcePublicAPI, - }; - mockDatasource.toExpression.mockReturnValue('datasource'); mockDatasource.getLayers.mockReturnValue(['first']); - const toExpression = jest.fn( - ( - state: unknown, - datasourceLayers: Record, - attributes?: Partial<{ title: string; description: string }> - ) => 'testVis' - ); - const mounted = await mountWithProvider( { }} framePublicAPI={framePublicAPI} visualizationMap={{ - testVis: { ...mockVisualization, toExpression }, + testVis: { ...mockVisualization, toExpression: () => 'testVis' }, }} ExpressionRenderer={expressionRendererMock} - />, - { - preloadedState: { - appliedState: { - activeDatasourceId: 'testDatasource', - visualization: { - activeId: 'testVis', - state: { applied: true }, - }, - datasourceStates: { - [mockDatasource.id]: { isLoading: false, state: { applied: true } }, - }, - }, - }, - } + /> ); instance = mounted.instance; - expect(toExpression).toHaveBeenCalled(); - const [appliedVisualizationState, appliedDatasourceLayers] = toExpression.mock.calls[0]; - expect((appliedVisualizationState as { applied: boolean }).applied).toBe(true); - expect( - (appliedDatasourceLayers?.first as DatasourcePublicAPI & { applied: boolean }).applied - ).toBe(true); + instance.update(); - // START NEW SCENARIO — make sure it switches back to using working state when auto-apply is reenabled + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={datasource} + | testVis" + `); - // have to do this part manually in the test, but in the application it will happen in the frame API selector which will be called in the parent component tree + act(() => { + mounted.lensStore.dispatch(disableAutoApply()); + }); + + mockDatasource.toExpression.mockReturnValue('new-datasource'); instance.setProps({ - framePublicAPI: { ...framePublicAPI, appliedDatasourceLayers: undefined }, + visualizationMap: { + testVis: { ...mockVisualization, toExpression: () => 'new-vis' }, + }, }); - toExpression.mockClear(); - mounted.lensStore.dispatch(enableAutoApply()); + // nothing should change + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={datasource} + | testVis" + `); + + act(() => { + mounted.lensStore.dispatch(applyChanges()); + }); + instance.update(); - expect(toExpression).toHaveBeenCalledTimes(1); - const [visualizationState, datasourceLayers] = toExpression.mock.calls[0]; - expect((visualizationState as { applied: boolean }).applied).toBeUndefined(); - expect( - (datasourceLayers?.first as DatasourcePublicAPI & { applied: boolean }).applied - ).toBeUndefined(); + // should update + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={new-datasource} + | new-vis" + `); }); it('should execute a trigger on expression event', async () => { @@ -371,6 +361,7 @@ describe('workspace_panel', () => { } ); instance = mounted.instance; + instance.update(); const ast = fromExpression(instance.find(expressionRendererMock).prop('expression') as string); @@ -654,6 +645,9 @@ describe('workspace_panel', () => { /> ); instance = mounted.instance; + act(() => { + instance.update(); + }); expect(instance.find('[data-test-subj="configuration-failure"]').exists()).toBeTruthy(); expect(instance.find(expressionRendererMock)).toHaveLength(0); diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index a99585cd8f4a2..3f455e4540f19 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -122,8 +122,6 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac ); }); -let lastApplyChangesCounter = 0; - // Exported for testing purposes only. export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ framePublicAPI, @@ -145,11 +143,15 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const autoApplyEnabled = useLensSelector(selectAutoApplyEnabled); const applyChangesCounter = useLensSelector(selectApplyChangesCounter); - const triggerApply = applyChangesCounter !== lastApplyChangesCounter; - lastApplyChangesCounter = applyChangesCounter; - const [expressionToRender, setExpressionToRender] = useState(); const [finishedInitialRender, setFinishedInitialRender] = useState(false); + const [lastApplyChangesCounter, setLastApplyChangesCounter] = useState(0); + + const triggerApply = applyChangesCounter !== lastApplyChangesCounter; + + if (triggerApply) { + setLastApplyChangesCounter(applyChangesCounter); + } const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender || triggerApply; @@ -373,7 +375,7 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }; const renderVisualization = () => { - if (!expressionExists) { + if (expression === null) { return renderEmptyWorkspace(); } return ( From 923bf57c4cfafb6560ad3a18e362708699f47d38 Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 15:38:38 -0600 Subject: [PATCH 06/10] refine workspace panel state usage --- .../workspace_panel/workspace_panel.tsx | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index 3f455e4540f19..b5880b2fff1ac 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -92,6 +92,8 @@ interface WorkspaceState { fixAction?: DatasourceFixAction; }>; expandError: boolean; + initialRenderComplete: boolean; + lastApplyChangesCounter: number; } const dropProps = { @@ -122,6 +124,8 @@ export const WorkspacePanel = React.memo(function WorkspacePanel(props: Workspac ); }); +let expressionToRender: null | undefined | string; + // Exported for testing purposes only. export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ framePublicAPI, @@ -143,24 +147,22 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const autoApplyEnabled = useLensSelector(selectAutoApplyEnabled); const applyChangesCounter = useLensSelector(selectApplyChangesCounter); - const [expressionToRender, setExpressionToRender] = useState(); - const [finishedInitialRender, setFinishedInitialRender] = useState(false); - const [lastApplyChangesCounter, setLastApplyChangesCounter] = useState(0); - - const triggerApply = applyChangesCounter !== lastApplyChangesCounter; - - if (triggerApply) { - setLastApplyChangesCounter(applyChangesCounter); - } - - const shouldApplyChanges = autoApplyEnabled || !finishedInitialRender || triggerApply; - const { datasourceLayers } = framePublicAPI; const [localState, setLocalState] = useState({ expressionBuildError: undefined, expandError: false, + lastApplyChangesCounter: 0, + initialRenderComplete: false, }); + const triggerApply = applyChangesCounter !== localState.lastApplyChangesCounter; + + if (triggerApply) { + setLocalState((s) => ({ ...s, lastApplyChangesCounter: applyChangesCounter })); + } + + const shouldApplyChanges = autoApplyEnabled || !localState.initialRenderComplete || triggerApply; + const activeVisualization = visualization.activeId ? visualizationMap[visualization.activeId] : null; @@ -256,19 +258,17 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ visualization.activeId, ]); - useEffect(() => { - if (shouldApplyChanges) { - setExpressionToRender(expression); - } - }, [expression, shouldApplyChanges]); + if (shouldApplyChanges) { + expressionToRender = expression; + } - useEffect(() => { + if (!autoApplyEnabled) { dispatchLens(setChangesApplied(expression === expressionToRender)); - }, [expression, dispatchLens, expressionToRender]); + } const expressionExists = Boolean(expressionToRender); - if (expressionExists && !finishedInitialRender) { - setFinishedInitialRender(true); + if (expressionExists && !localState.initialRenderComplete) { + setLocalState((s) => ({ ...s, initialRenderComplete: true })); } useEffect(() => { From 6d8b1d3943269c8a1da83a09b81c6d90cd4bdbf4 Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 20:43:22 -0600 Subject: [PATCH 07/10] unskip auto-apply test --- .../workspace_panel/workspace_panel.test.tsx | 46 ++++++++++++++----- .../workspace_panel/workspace_panel.tsx | 23 ++++++---- .../public/state_management/lens_slice.ts | 4 +- .../lens/public/state_management/selectors.ts | 2 +- .../lens/public/state_management/types.ts | 2 +- 5 files changed, 54 insertions(+), 23 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index d7f45930d7676..d4fe833042031 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -186,7 +186,7 @@ describe('workspace_panel', () => { `); }); - it.skip('should give user control when auto-apply disabled', async () => { + it('should give user control when auto-apply disabled', async () => { const framePublicAPI = createMockFramePublicAPI(); framePublicAPI.datasourceLayers = { first: mockDatasource.publicAPIMock, @@ -205,23 +205,25 @@ describe('workspace_panel', () => { testVis: { ...mockVisualization, toExpression: () => 'testVis' }, }} ExpressionRenderer={expressionRendererMock} - /> + />, + { + preloadedState: { + autoApplyDisabled: true, + }, + } ); instance = mounted.instance; instance.update(); + // allows initial render expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` "kibana | lens_merge_tables layerIds=\\"first\\" tables={datasource} | testVis" `); - act(() => { - mounted.lensStore.dispatch(disableAutoApply()); - }); - mockDatasource.toExpression.mockReturnValue('new-datasource'); instance.setProps({ visualizationMap: { @@ -236,9 +238,7 @@ describe('workspace_panel', () => { | testVis" `); - act(() => { - mounted.lensStore.dispatch(applyChanges()); - }); + mounted.lensStore.dispatch(applyChanges()); instance.update(); // should update @@ -247,6 +247,30 @@ describe('workspace_panel', () => { | lens_merge_tables layerIds=\\"first\\" tables={new-datasource} | new-vis" `); + + mockDatasource.toExpression.mockReturnValue('other-new-datasource'); + instance.setProps({ + visualizationMap: { + testVis: { ...mockVisualization, toExpression: () => 'other-new-vis' }, + }, + }); + + // should not update + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={new-datasource} + | new-vis" + `); + + mounted.lensStore.dispatch(enableAutoApply()); + instance.update(); + + // reenabling auto-apply triggers an update as well + expect(instance.find(expressionRendererMock).prop('expression')).toMatchInlineSnapshot(` + "kibana + | lens_merge_tables layerIds=\\"first\\" tables={other-new-datasource} + | other-new-vis" + `); }); it('should execute a trigger on expression event', async () => { @@ -718,7 +742,7 @@ describe('workspace_panel', () => { expect(instance.find(expressionRendererMock)).toHaveLength(0); }); - it('should NOT display errors for unapplied changes', async () => { + it.skip('should NOT display errors for unapplied changes', async () => { // this test is important since we don't want the workspace panel to // display errors if the user has disabled auto-apply, messed something up, // but not yet applied their changes @@ -773,7 +797,7 @@ describe('workspace_panel', () => { expect(showingErrors()).toBeFalsy(); - // introduce some issues in working state + // introduce some issues lensStore.dispatch( updateDatasourceState({ datasourceId: 'testDatasource', diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index b5880b2fff1ac..fb657f09bf2cc 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -68,7 +68,6 @@ import { selectSearchSessionId, selectAutoApplyEnabled, selectApplyChangesCounter, - selectTriggerApplyChanges, } from '../../../state_management'; import type { LensInspector } from '../../../lens_inspector_service'; import { inferTimeField } from '../../../utils'; @@ -147,7 +146,6 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ const autoApplyEnabled = useLensSelector(selectAutoApplyEnabled); const applyChangesCounter = useLensSelector(selectApplyChangesCounter); - const { datasourceLayers } = framePublicAPI; const [localState, setLocalState] = useState({ expressionBuildError: undefined, expandError: false, @@ -155,13 +153,22 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ initialRenderComplete: false, }); + useEffect(() => { + return () => { + expressionToRender = null; + }; + }, []); + const triggerApply = applyChangesCounter !== localState.lastApplyChangesCounter; if (triggerApply) { setLocalState((s) => ({ ...s, lastApplyChangesCounter: applyChangesCounter })); } - const shouldApplyChanges = autoApplyEnabled || !localState.initialRenderComplete || triggerApply; + const shouldApplyExpression = + autoApplyEnabled || !localState.initialRenderComplete || triggerApply; + + const { datasourceLayers } = framePublicAPI; const activeVisualization = visualization.activeId ? visualizationMap[visualization.activeId] @@ -206,7 +213,7 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ [activeVisualization, visualization.state, activeDatasourceId, datasourceMap, datasourceStates] ); - const expression = useMemo(() => { + const _expression = useMemo(() => { if (!configurationValidationError?.length && !missingRefsErrors.length && !unknownVisError) { try { const ast = buildExpression({ @@ -258,12 +265,12 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ visualization.activeId, ]); - if (shouldApplyChanges) { - expressionToRender = expression; + if (shouldApplyExpression) { + expressionToRender = _expression; } if (!autoApplyEnabled) { - dispatchLens(setChangesApplied(expression === expressionToRender)); + dispatchLens(setChangesApplied(_expression === expressionToRender)); } const expressionExists = Boolean(expressionToRender); @@ -375,7 +382,7 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ }; const renderVisualization = () => { - if (expression === null) { + if (expressionToRender === null) { return renderEmptyWorkspace(); } return ( diff --git a/x-pack/plugins/lens/public/state_management/lens_slice.ts b/x-pack/plugins/lens/public/state_management/lens_slice.ts index af43af1f145b3..157b700ccba58 100644 --- a/x-pack/plugins/lens/public/state_management/lens_slice.ts +++ b/x-pack/plugins/lens/public/state_management/lens_slice.ts @@ -211,10 +211,10 @@ export const makeLensReducer = (storeDeps: LensStoreDeps) => { }; }, [enableAutoApply.type]: (state) => { - state.applyChangesDisabled = false; + state.autoApplyDisabled = false; }, [disableAutoApply.type]: (state) => { - state.applyChangesDisabled = true; + state.autoApplyDisabled = true; }, [applyChanges.type]: (state) => { if (typeof state.applyChangesCounter === 'undefined') { diff --git a/x-pack/plugins/lens/public/state_management/selectors.ts b/x-pack/plugins/lens/public/state_management/selectors.ts index 365e05a282014..3b6f9100bfaef 100644 --- a/x-pack/plugins/lens/public/state_management/selectors.ts +++ b/x-pack/plugins/lens/public/state_management/selectors.ts @@ -19,7 +19,7 @@ export const selectFilters = (state: LensState) => state.lens.filters; export const selectResolvedDateRange = (state: LensState) => state.lens.resolvedDateRange; export const selectVisualization = (state: LensState) => state.lens.visualization; export const selectStagedPreview = (state: LensState) => state.lens.stagedPreview; -export const selectAutoApplyEnabled = (state: LensState) => !state.lens.applyChangesDisabled; +export const selectAutoApplyEnabled = (state: LensState) => !state.lens.autoApplyDisabled; export const selectChangesApplied = (state: LensState) => Boolean(state.lens.changesApplied); export const selectApplyChangesCounter = (state: LensState) => state.lens.applyChangesCounter || 0; export const selectDatasourceStates = (state: LensState) => state.lens.datasourceStates; diff --git a/x-pack/plugins/lens/public/state_management/types.ts b/x-pack/plugins/lens/public/state_management/types.ts index 256100a2287ff..0c902f944072d 100644 --- a/x-pack/plugins/lens/public/state_management/types.ts +++ b/x-pack/plugins/lens/public/state_management/types.ts @@ -33,7 +33,7 @@ export interface PreviewState { export interface EditorFrameState extends PreviewState { activeDatasourceId: string | null; stagedPreview?: PreviewState; - applyChangesDisabled?: boolean; + autoApplyDisabled?: boolean; applyChangesCounter?: number; changesApplied?: boolean; isFullscreenDatasource?: boolean; From 806b6d7249ea9514e28af6164d5b80a3183d93a6 Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Thu, 24 Feb 2022 20:57:28 -0600 Subject: [PATCH 08/10] remove missing dependency --- .../editor_frame/workspace_panel/workspace_panel.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index d4fe833042031..7205210bca863 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -8,7 +8,7 @@ import React from 'react'; import { act } from 'react-dom/test-utils'; import { ReactExpressionRendererProps } from '../../../../../../../src/plugins/expressions/public'; -import { DatasourcePublicAPI, FramePublicAPI, Visualization } from '../../../types'; +import { FramePublicAPI, Visualization } from '../../../types'; import { createMockVisualization, createMockDatasource, From a21de0074d584e6ca1b62d1bd7b4fb5981fe66be Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Fri, 25 Feb 2022 13:47:39 -0600 Subject: [PATCH 09/10] allow null expression as initial render (empty workspace) --- .../editor_frame/workspace_panel/workspace_panel.test.tsx | 2 +- .../editor_frame/workspace_panel/workspace_panel.tsx | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index 7205210bca863..05e60474c61fb 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -742,7 +742,7 @@ describe('workspace_panel', () => { expect(instance.find(expressionRendererMock)).toHaveLength(0); }); - it.skip('should NOT display errors for unapplied changes', async () => { + it('should NOT display errors for unapplied changes', async () => { // this test is important since we don't want the workspace panel to // display errors if the user has disabled auto-apply, messed something up, // but not yet applied their changes diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index fb657f09bf2cc..81a3990fdd5fd 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -274,7 +274,8 @@ export const InnerWorkspacePanel = React.memo(function InnerWorkspacePanel({ } const expressionExists = Boolean(expressionToRender); - if (expressionExists && !localState.initialRenderComplete) { + // null signals an empty workspace which should count as an initial render + if ((expressionExists || expressionToRender === null) && !localState.initialRenderComplete) { setLocalState((s) => ({ ...s, initialRenderComplete: true })); } From cf332d6d1b0208a03f7c001b7b55e63ea6464f1b Mon Sep 17 00:00:00 2001 From: Andrew Tate Date: Fri, 25 Feb 2022 14:29:46 -0600 Subject: [PATCH 10/10] add empty workspace test --- .../workspace_panel/workspace_panel.test.tsx | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index 05e60474c61fb..afee50de06675 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -273,6 +273,36 @@ describe('workspace_panel', () => { `); }); + it('should allow empty workspace as initial render when auto-apply disabled', async () => { + mockVisualization.toExpression.mockReturnValue('testVis'); + const framePublicAPI = createMockFramePublicAPI(); + framePublicAPI.datasourceLayers = { + first: mockDatasource.publicAPIMock, + }; + + const mounted = await mountWithProvider( + , + { + preloadedState: { + autoApplyDisabled: true, + }, + } + ); + + instance = mounted.instance; + + expect(instance.exists('[data-test-subj="empty-workspace"]')).toBeTruthy(); + }); + it('should execute a trigger on expression event', async () => { const framePublicAPI = createMockFramePublicAPI(); framePublicAPI.datasourceLayers = {