A number of NB changes

- Fixed a subtle serialisation bug with on_failure in options - Fixed a performance bug by using useRef in pipeline form - Memoized the drag and drop tree - Removed use of "isValid" from form lib... I think this should be removed entirely.
jloleysens · May 1, 2020 · 62b0f53 · 62b0f53
1 parent b8b169f
commit 62b0f53
Show file tree

Hide file tree

Showing 8 changed files with 99 additions and 83 deletions.
diff --git a/...ck/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx b/...ck/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx
@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { useState, useCallback } from 'react';
+import React, { useState, useCallback, useRef } from 'react';
 import { FormattedMessage } from '@kbn/i18n/react';
 import { EuiButton, EuiButtonEmpty, EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui';
 
@@ -45,7 +45,7 @@ export const PipelineForm: React.FunctionComponent<PipelineFormProps> = ({
 
   const [isTestingPipeline, setIsTestingPipeline] = useState<boolean>(false);
 
-  const [processorsEditorState, setProcessorsEditorState] = useState<OnUpdateHandlerArg>();
+  const processorStateRef = useRef<OnUpdateHandlerArg>();
 
   const handleSave: FormConfig['onSubmit'] = async (formData, isValid) => {
     let override: any = {};
@@ -54,9 +54,10 @@ export const PipelineForm: React.FunctionComponent<PipelineFormProps> = ({
       return;
     }
 
-    if (processorsEditorState) {
-      if (processorsEditorState.isValid === undefined && (await processorsEditorState.validate())) {
-        override = processorsEditorState.getData();
+    if (processorStateRef.current) {
+      const processorsState = processorStateRef.current;
+      if (await processorsState.validate()) {
+        override = processorsState.getData();
       } else {
         return;
       }
@@ -93,8 +94,8 @@ export const PipelineForm: React.FunctionComponent<PipelineFormProps> = ({
   );
 
   const onProcessorsChangeHandler = useCallback<OnUpdateHandler>(
-    arg => setProcessorsEditorState(arg),
-    [setProcessorsEditorState]
+    arg => (processorStateRef.current = arg),
+    []
   );
 
   return (

diff --git a/...omponents/pipeline_processors_editor/components/drag_and_drop_tree/drag_and_drop_tree.tsx b/...omponents/pipeline_processors_editor/components/drag_and_drop_tree/drag_and_drop_tree.tsx
@@ -4,12 +4,12 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { FunctionComponent } from 'react';
+import React, { FunctionComponent, useState, memo } from 'react';
 import { EuiDragDropContext, EuiDroppable } from '@elastic/eui';
 
 import { ProcessorInternal, DraggableLocation, ProcessorSelector } from '../../types';
 
-import { mapDestinationIndexToTreeLocation } from './utils';
+import { resolveDestinationLocation } from './utils';
 
 import { TreeNode, TreeNodeComponentArgs } from './tree_node';
 
@@ -21,20 +21,31 @@ interface OnDragEndArgs {
 export interface Props {
   processors: ProcessorInternal[];
   onDragEnd: (args: OnDragEndArgs) => void;
-  nodeComponent: (arg: TreeNodeComponentArgs) => React.ReactNode;
+  renderItem: (arg: TreeNodeComponentArgs) => React.ReactNode;
 }
 
 /** This value comes from the {@link ProcessorInternal} type */
 const ON_FAILURE = 'onFailure';
 
-export const DragAndDropTree: FunctionComponent<Props> = ({
+/**
+ * Takes in array of {@link ProcessorInternal} and renders a drag and drop tree.
+ *
+ * @remark
+ * Because of issues with nesting EuiDroppable (based on react-beautiful-dnd) we render
+ * a flat structure with one droppable. This component is responsible for maintaining the
+ * {@link ProcessorSelector}s back to the nested structure so that it can emit instructions
+ * the reducer will understand.
+ */
+export const DragAndDropTreeUI: FunctionComponent<Props> = ({
   processors,
   onDragEnd,
-  nodeComponent,
+  renderItem,
 }) => {
   let flatTreeIndex = 0;
   const items: Array<[ProcessorSelector, React.ReactElement]> = [];
 
+  const [currentDragSelector, setCurrentDragSelector] = useState<string | undefined>();
+
   const addRenderedItems = (
     _processors: ProcessorInternal[],
     _selector: ProcessorSelector,
@@ -51,11 +62,11 @@ export const DragAndDropTree: FunctionComponent<Props> = ({
           level={level}
           processor={processor}
           selector={nodeSelector}
-          component={nodeComponent}
+          component={renderItem}
         />,
       ]);
 
-      if (processor.onFailure?.length) {
+      if (processor.onFailure?.length && nodeSelector.join('.') !== currentDragSelector) {
         addRenderedItems(processor.onFailure, nodeSelector.concat(ON_FAILURE), level + 1);
       }
     });
@@ -65,7 +76,13 @@ export const DragAndDropTree: FunctionComponent<Props> = ({
 
   return (
     <EuiDragDropContext
-      onDragEnd={({ source, destination, combine }) => {
+      onBeforeCapture={({ draggableId: serializedSelector }) => {
+        setCurrentDragSelector(serializedSelector);
+      }}
+      onDragEnd={arg => {
+        setCurrentDragSelector(undefined);
+
+        const { source, destination, combine } = arg;
         if (source && combine) {
           const [sourceSelector] = items[source.index];
           const destinationSelector = combine.draggableId.split('.');
@@ -89,7 +106,7 @@ export const DragAndDropTree: FunctionComponent<Props> = ({
               index: parseInt(sourceSelector[sourceSelector.length - 1], 10),
               selector: sourceSelector.slice(0, -1),
             },
-            destination: mapDestinationIndexToTreeLocation(
+            destination: resolveDestinationLocation(
               items.map(([selector]) => selector),
               !sourceSelector.slice(0, -1).length,
               destination.index
@@ -104,3 +121,7 @@ export const DragAndDropTree: FunctionComponent<Props> = ({
     </EuiDragDropContext>
   );
 };
+
+export const DragAndDropTree = memo(DragAndDropTreeUI, (prev, current) => {
+  return prev.processors === current.processors && prev.onDragEnd === current.onDragEnd;
+});
diff --git a/...ication/components/pipeline_processors_editor/components/drag_and_drop_tree/tree_node.tsx b/...ication/components/pipeline_processors_editor/components/drag_and_drop_tree/tree_node.tsx
@@ -32,23 +32,21 @@ export const TreeNode: FunctionComponent<Props> = ({
 }) => {
   const id = selector.join('.');
   return (
-    <>
-      <EuiDraggable spacing="l" draggableId={id} key={id} index={index} customDragHandle={true}>
-        {provided => (
-          <EuiPanel style={{ marginLeft: 30 * level + 'px' }} paddingSize="s">
-            <EuiFlexGroup gutterSize="none" direction="column" alignItems="flexStart">
-              <EuiFlexGroup gutterSize="s" alignItems="center">
-                <EuiFlexItem grow={false}>
-                  <div {...provided.dragHandleProps}>
-                    <EuiIcon type="grab" />
-                  </div>
-                </EuiFlexItem>
-                <EuiFlexItem grow={false}>{component({ processor, selector })}</EuiFlexItem>
-              </EuiFlexGroup>
+    <EuiDraggable spacing="l" draggableId={id} key={id} index={index} customDragHandle={true}>
+      {provided => (
+        <EuiPanel style={{ marginLeft: 30 * level + 'px' }} paddingSize="s">
+          <EuiFlexGroup gutterSize="none" direction="column" alignItems="flexStart">
+            <EuiFlexGroup gutterSize="s" alignItems="center">
+              <EuiFlexItem grow={false}>
+                <div {...provided.dragHandleProps}>
+                  <EuiIcon type="grab" />
+                </div>
+              </EuiFlexItem>
+              <EuiFlexItem grow={false}>{component({ processor, selector })}</EuiFlexItem>
             </EuiFlexGroup>
-          </EuiPanel>
-        )}
-      </EuiDraggable>
-    </>
+          </EuiFlexGroup>
+        </EuiPanel>
+      )}
+    </EuiDraggable>
   );
 };
diff --git a/.../application/components/pipeline_processors_editor/components/drag_and_drop_tree/utils.ts b/.../application/components/pipeline_processors_editor/components/drag_and_drop_tree/utils.ts
@@ -6,7 +6,7 @@
 
 import { DraggableLocation, ProcessorSelector } from '../../types';
 
-export const mapDestinationIndexToTreeLocation = (
+export const resolveDestinationLocation = (
   items: ProcessorSelector[],
   isRootLevelSource: boolean,
   destinationIndex: number

diff --git a/...gins/ingest_pipelines/public/application/components/pipeline_processors_editor/data_in.ts b/...gins/ingest_pipelines/public/application/components/pipeline_processors_editor/data_in.ts
@@ -23,10 +23,10 @@ const getProcessorType = (processor: Processor): string => {
 
 const convertToPipelineInternalProcessor = (processor: Processor): ProcessorInternal => {
   const type = getProcessorType(processor);
-  const options = processor[type];
-  const onFailure = options.on_failure?.length
-    ? convertProcessors(options.on_failure)
-    : (options.on_failure as ProcessorInternal[] | undefined);
+  const { on_failure: originalOnFailure, ...options } = processor[type];
+  const onFailure = originalOnFailure?.length
+    ? convertProcessors(originalOnFailure)
+    : (originalOnFailure as ProcessorInternal[] | undefined);
   return {
     type,
     onFailure,

diff --git a/...s/public/application/components/pipeline_processors_editor/pipeline_processors_editor.tsx b/...s/public/application/components/pipeline_processors_editor/pipeline_processors_editor.tsx
@@ -9,16 +9,20 @@ import { EuiButton } from '@elastic/eui';
 
 import { Processor } from '../../../../common/types';
 
+import { OnFormUpdateArg } from '../../../shared_imports';
+
 import { SettingsFormFlyout, DragAndDropTree, PipelineProcessorEditorItem } from './components';
 import { deserialize } from './data_in';
 import { serialize, SerializeResult } from './data_out';
-import { useProcessorsState } from './reducer';
+import { useProcessorsState } from './processors_reducer';
 import { ProcessorInternal, ProcessorSelector } from './types';
 
-export interface OnUpdateHandlerArg {
+interface FormState {
+  validate: OnFormUpdateArg<any>['validate'];
+}
+
+export interface OnUpdateHandlerArg extends FormState {
   getData: () => SerializeResult;
-  validate: () => Promise<boolean>;
-  isValid?: boolean;
 }
 
 export type OnUpdateHandler = (arg: OnUpdateHandlerArg) => void;
@@ -56,21 +60,27 @@ export const PipelineProcessorsEditor: FunctionComponent<Props> = ({
   const [state, dispatch] = useProcessorsState(dataInResult);
   const { processors } = state;
 
-  useEffect(() => {
-    onUpdate({
-      isValid: state.isValid,
-      validate: state.validate,
-      getData: () => serialize(state),
-    });
-  }, [state, onUpdate]);
+  const [formState, setFormState] = useState<FormState>({
+    validate: () => Promise.resolve(true),
+  });
 
   const onFormUpdate = useCallback(
     arg => {
-      dispatch({ type: 'processorForm.update', payload: arg });
+      setFormState({ validate: arg.validate });
     },
-    [dispatch]
+    [setFormState]
   );
 
+  useEffect(() => {
+    onUpdate({
+      validate: async () => {
+        const formValid = await formState.validate();
+        return formValid && mode.id === 'idle';
+      },
+      getData: () => serialize(state),
+    });
+  }, [state, onUpdate, formState, mode]);
+
   const onSubmit = useCallback(
     processorTypeAndOptions => {
       switch (mode.id) {
@@ -108,21 +118,26 @@ export const PipelineProcessorsEditor: FunctionComponent<Props> = ({
     [dispatch, mode]
   );
 
+  const onDragEnd = useCallback(
+    args => {
+      dispatch({
+        type: 'moveProcessor',
+        payload: args,
+      });
+    },
+    [dispatch]
+  );
+
   const dismissFlyout = () => {
     setMode({ id: 'idle' });
   };
 
   return (
     <>
       <DragAndDropTree
-        onDragEnd={args => {
-          dispatch({
-            type: 'moveProcessor',
-            payload: args,
-          });
-        }}
+        onDragEnd={onDragEnd}
         processors={processors}
-        nodeComponent={({ processor, selector }) => (
+        renderItem={({ processor, selector }) => (
           <PipelineProcessorEditorItem
             onClick={type => {
               switch (type) {
@@ -156,7 +171,7 @@ export const PipelineProcessorsEditor: FunctionComponent<Props> = ({
           processor={mode.id === 'editingProcessor' ? mode.arg.processor : undefined}
           onClose={() => {
             dismissFlyout();
-            dispatch({ type: 'processorForm.close' });
+            setFormState({ validate: () => Promise.resolve(true) });
           }}
         />
       ) : (

diff --git a/...ipeline_processors_editor/reducer.test.ts → ...cessors_editor/processors_reducer.test.ts b/...ipeline_processors_editor/reducer.test.ts → ...cessors_editor/processors_reducer.test.ts
@@ -4,11 +4,10 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { reducer, State } from './reducer';
+import { reducer, State } from './processors_reducer';
 
 const initialState: State = {
   processors: [],
-  validate: () => Promise.resolve(true),
 };
 
 describe('Processors reducer', () => {

diff --git a/...nts/pipeline_processors_editor/reducer.ts → ...e_processors_editor/processors_reducer.ts b/...nts/pipeline_processors_editor/reducer.ts → ...e_processors_editor/processors_reducer.ts
@@ -6,18 +6,11 @@
 import { Reducer, useReducer } from 'react';
 import { euiDragDropReorder } from '@elastic/eui';
 
-import { OnFormUpdateArg } from '../../../shared_imports';
-
 import { DeserializeResult } from './data_in';
 import { getValue, setValue, unsafeProcessorMove, PARENT_CHILD_NEST_ERROR } from './utils';
 import { ProcessorInternal, DraggableLocation, ProcessorSelector } from './types';
 
-type StateArg = DeserializeResult;
-
-export interface State extends StateArg {
-  isValid?: boolean;
-  validate: () => Promise<boolean>;
-}
+export type State = DeserializeResult;
 
 type Action =
   | {
@@ -42,10 +35,7 @@ type Action =
   | {
       type: 'moveProcessor';
       payload: { source: DraggableLocation; destination: DraggableLocation };
-    }
-  // Does not quite belong here, but using in reducer for convenience
-  | { type: 'processorForm.update'; payload: OnFormUpdateArg<any> }
-  | { type: 'processorForm.close' };
+    };
 
 const addSelectorRoot = (selector: ProcessorSelector): ProcessorSelector => {
   return ['processors'].concat(selector);
@@ -126,16 +116,8 @@ export const reducer: Reducer<State, Action> = (state, action) => {
     return setValue(processorsSelector, state, processors);
   }
 
-  if (action.type === 'processorForm.update') {
-    return { ...state, ...action.payload };
-  }
-
-  if (action.type === 'processorForm.close') {
-    return { ...state, isValid: undefined, validate: () => Promise.resolve(true) };
-  }
-
   return state;
 };
 
-export const useProcessorsState = (initialState: StateArg) =>
-  useReducer<typeof reducer>(reducer, { ...initialState, validate: () => Promise.resolve(true) });
+export const useProcessorsState = (initialState: State) =>
+  useReducer<typeof reducer>(reducer, { ...initialState });