Skip to content

[lens] Allow updater function to be used for updating state #43373

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
flash1293 merged 3 commits into from
Aug 20, 2019
Merged

[lens] Allow updater function to be used for updating state #43373

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce4665c
[lens] Allow updater function to be used for updating state
wylieconlon Aug 15, 2019
172dada
Fix type errors
wylieconlon Aug 15, 2019
98bdd07
Don't use StateSetter for visualization yet
wylieconlon Aug 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/data_panel_wrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { DatasourceDataPanelProps, Datasource } from '../../../public';
import { NativeRenderer } from '../../native_renderer';
import { Action } from './state_management';
import { DragContext } from '../../drag_drop';
import { StateSetter } from '../../types';

interface DataPanelWrapperProps {
datasourceState: unknown;
Expand All @@ -21,11 +22,11 @@ interface DataPanelWrapperProps {
}

export const DataPanelWrapper = memo((props: DataPanelWrapperProps) => {
const setDatasourceState = useMemo(
() => (newState: unknown) => {
const setDatasourceState: StateSetter<unknown> = useMemo(
() => updater => {
props.dispatch({
type: 'UPDATE_DATASOURCE_STATE',
newState,
updater,

@chrisdavies chrisdavies Aug 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right. Actions are supposed to be serializable / deserializable. It's one of the benefits of using an action / reducer pattern.

I'm not sure what we should do here, but I'd like to think it through a bit more. If we ever end up using a reducer-based undo / redo system, it would be really nice to keep actions data-only.

@wylieconlon wylieconlon Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be right in the world of redux, but I'm not sure that pattern applies in the build-in dispatcher.

Also, this concept is already something we merged for updating layers: https://github.com/elastic/kibana/blob/feature/lens/x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/state_management.ts#L44

@flash1293 flash1293 Aug 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also not a strict rule in redux itself (https://redux.js.org/faq/actions#why-should-type-be-a-string-or-at-least-serializable-why-should-my-action-types-be-constants), but you are right, we should be aware of the trade-offs. I thought about this and discussed with @streamich when doing this change to the layer state management and I think it's the right thing to do this here.

For the undo / redo system the easiest way to is to buffer the sequence of states in a separate property (like this: https://redux.js.org/recipes/implementing-undo-history) to be able to re-apply them. The actions don't have to be serializable for this to work because they are still only applied once. Another way out of this would be to keep the actions as they are for the dispatch calls and call the updater functions in a middleware instead of the actual reducer to keep it serializable (just like thunk and saga works). But I would only do that once we get to the point we actually need it. We should take the easiest way that doesn't paint us in a corner here IMHO.

datasourceId: props.activeDatasource!,
});
},
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/editor_frame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function EditorFrame(props: EditorFrameProps) {
.then(datasourceState => {
dispatch({
type: 'UPDATE_DATASOURCE_STATE',
newState: datasourceState,
updater: datasourceState,
datasourceId,
});
})
Expand All @@ -84,7 +84,7 @@ export function EditorFrame(props: EditorFrameProps) {
dispatch({
type: 'UPDATE_DATASOURCE_STATE',
datasourceId: id,
newState,
updater: newState,
});
},
layer
Expand Down
21 changes: 10 additions & 11 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/state_management.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ describe('editor_frame state management', () => {
expect(newState.visualization.state).toBe(newVisState);
});

it('should update the datasource state on update', () => {
const newDatasourceState = {};
it('should update the datasource state with passed in reducer', () => {
const datasourceReducer = jest.fn(() => ({ changed: true }));
const newState = reducer(
{
datasourceStates: {
Expand All @@ -143,16 +143,17 @@ describe('editor_frame state management', () => {
},
{
type: 'UPDATE_DATASOURCE_STATE',
newState: newDatasourceState,
updater: datasourceReducer,
datasourceId: 'testDatasource',
}
);

expect(newState.datasourceStates.testDatasource.state).toBe(newDatasourceState);
expect(newState.datasourceStates.testDatasource.state).toEqual({ changed: true });
expect(datasourceReducer).toHaveBeenCalledTimes(1);
});

it('should update the datasource state with passed in reducer', () => {
const layerReducer = jest.fn((_state, layerId) => ({ inserted: layerId }));
it('should update the layer state with passed in reducer', () => {
const newDatasourceState = {};
const newState = reducer(
{
datasourceStates: {
Expand All @@ -169,15 +170,13 @@ describe('editor_frame state management', () => {
},
},
{
type: 'UPDATE_LAYER',
layerId: 'abc',
updater: layerReducer,
type: 'UPDATE_DATASOURCE_STATE',
updater: newDatasourceState,
datasourceId: 'testDatasource',
}
);

expect(newState.datasourceStates.testDatasource.state).toEqual({ inserted: 'abc' });
expect(layerReducer).toHaveBeenCalledTimes(1);
expect(newState.datasourceStates.testDatasource.state).toBe(newDatasourceState);
});

it('should should switch active visualization', () => {
Expand Down
7 changes: 5 additions & 2 deletions x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/state_management.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export type Action =
}
| {
type: 'UPDATE_DATASOURCE_STATE';
newState: unknown;
updater: unknown | ((prevState: unknown) => unknown);
datasourceId: string;
}
| {
Expand Down Expand Up @@ -168,7 +168,10 @@ export const reducer = (state: EditorFrameState, action: Action): EditorFrameSta
datasourceStates: {
...state.datasourceStates,
[action.datasourceId]: {
state: action.newState,
state:
typeof action.updater === 'function'
? action.updater(state.datasourceStates[action.datasourceId].state)
: action.updater,
isLoading: false,
},
},
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/dimension_panel/dimension_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { EuiButtonIcon } from '@elastic/eui';
import { Storage } from 'ui/storage';
import { i18n } from '@kbn/i18n';
import { UiSettingsClientContract } from 'src/core/public';
import { DatasourceDimensionPanelProps } from '../../types';
import { DatasourceDimensionPanelProps, StateSetter } from '../../types';
import {
IndexPatternColumn,
IndexPatternPrivateState,
Expand All @@ -26,7 +26,7 @@ import { isDraggedField } from '../utils';

export type IndexPatternDimensionPanelProps = DatasourceDimensionPanelProps & {
state: IndexPatternPrivateState;
setState: (newState: IndexPatternPrivateState) => void;
setState: StateSetter<IndexPatternPrivateState>;
dragDropContext: DragContextState;
uiSettings: UiSettingsClientContract;
storage: Storage;
Expand Down
7 changes: 2 additions & 5 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/indexpattern.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ export function getIndexPatternDatasource({
<I18nProvider>
<IndexPatternDimensionPanel
state={state}
setState={newState => setState(newState)}
setState={setState}
uiSettings={uiSettings}
storage={storage}
layerId={props.layerId}
Expand All @@ -317,10 +317,7 @@ export function getIndexPatternDatasource({
},

renderLayerPanel: (domElement: Element, props: DatasourceLayerPanelProps) => {
render(
<LayerPanel state={state} setState={newState => setState(newState)} {...props} />,
domElement
);
render(<LayerPanel state={state} setState={setState} {...props} />, domElement);
},

removeColumnInTableSpec: (columnId: string) => {
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/layerpanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ import {
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { I18nProvider } from '@kbn/i18n/react';
import { DatasourceLayerPanelProps } from '../types';
import { DatasourceLayerPanelProps, StateSetter } from '../types';
import { IndexPatternPrivateState, IndexPatternLayer } from './indexpattern';
import { isLayerTransferable, updateLayerIndexPattern } from './state_helpers';

export interface IndexPatternLayerPanelProps extends DatasourceLayerPanelProps {
state: IndexPatternPrivateState;
setState: (newState: IndexPatternPrivateState) => void;
setState: StateSetter<IndexPatternPrivateState>;
}

function LayerPanelChooser({
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import _ from 'lodash';
import { Storage } from 'ui/storage';
import { UiSettingsClientContract } from 'src/core/public';
import { DimensionPriority, OperationMetadata } from '../types';
import { DimensionPriority, OperationMetadata, StateSetter } from '../types';
import {
IndexPatternColumn,
IndexPatternField,
Expand Down Expand Up @@ -60,7 +60,7 @@ export function getOperations(): OperationType[] {

export interface ParamEditorProps {
state: IndexPatternPrivateState;
setState: (newState: IndexPatternPrivateState) => void;
setState: StateSetter<IndexPatternPrivateState>;
columnId: string;
layerId: string;
uiSettings: UiSettingsClientContract;
Expand Down
8 changes: 5 additions & 3 deletions x-pack/legacy/plugins/lens/public/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ export interface DatasourceMetaData {
filterableIndexPatterns: Array<{ id: string; title: string }>;
}

export type StateSetter<T> = (newState: T | ((prevState: T) => T)) => void;

/**
* Interface for the datasource registry
*/
Expand All @@ -88,7 +90,7 @@ export interface Datasource<T = unknown, P = unknown> {
getDatasourceSuggestionsForField: (state: T, field: unknown) => Array<DatasourceSuggestion<T>>;
getDatasourceSuggestionsFromCurrentState: (state: T) => Array<DatasourceSuggestion<T>>;

getPublicAPI: (state: T, setState: (newState: T) => void, layerId: string) => DatasourcePublicAPI;
getPublicAPI: (state: T, setState: StateSetter<T>, layerId: string) => DatasourcePublicAPI;
}

/**
Expand Down Expand Up @@ -118,7 +120,7 @@ export type TableSpec = TableSpecColumn[];
export interface DatasourceDataPanelProps<T = unknown> {
state: T;
dragDropContext: DragContextState;
setState: (newState: T) => void;
setState: StateSetter<T>;
}

// The only way a visualization has to restrict the query building
Expand Down Expand Up @@ -171,7 +173,7 @@ export interface VisualizationProps<T = unknown> {
dragDropContext: DragContextState;
frame: FramePublicAPI;
state: T;
setState: (newState: T) => void;
setState: StateSetter<T>;

@flash1293 flash1293 Aug 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visualizations are also getting the new StateSetter interface but it's not handled accordingly in the state_management. Am I missing something here?

@wylieconlon wylieconlon Aug 16, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I was not intending to make any change to visualizations in this PR

}

export interface SuggestionRequest<T = unknown> {
Expand Down