[embeddable rebuild] useBatchedOptionalPublishingSubjects (elastic#18…

…0221) Fixes elastic#180088 1. Changes observable pipe from `debounceTime(), skip(1)` to `skip(1), debounceTime()`. Updates test to verify this change results in subscription getting fired when observable.next is called before debounceTime fires. 2. rename `useBatchedPublishingSubjects` to `useBatchedOptionalPublishingSubjects`. Remove `useMemo` since spreading subjects results in new array every time and useMemo does nothing. 3. Update `PresentationPanelInternal` to use `useBatchedOptionalPublishingSubjects` 4. create new `useBatchedPublishingSubjects` that types subjects as `PublishingSubject[]`. New --------- Co-authored-by: Kibana Machine <[email protected]>
peteharverson · Apr 10, 2024 · 83cb0e4 · 83cb0e4
1 parent d42c4bb
commit 83cb0e4
Show file tree

Hide file tree

Showing 7 changed files with 128 additions and 39 deletions.
diff --git a/packages/presentation/presentation_publishing/index.ts b/packages/presentation/presentation_publishing/index.ts
@@ -111,6 +111,7 @@ export {
 } from './interfaces/titles/publishes_panel_title';
 export { initializeTitles, type SerializedTitles } from './interfaces/titles/titles_api';
 export {
+  useBatchedOptionalPublishingSubjects,
   useBatchedPublishingSubjects,
   usePublishingSubject,
   useStateFromPublishingSubject,

diff --git a/packages/presentation/presentation_publishing/publishing_subject/index.ts b/packages/presentation/presentation_publishing/publishing_subject/index.ts
@@ -6,7 +6,10 @@
  * Side Public License, v 1.
  */
 
-export { useBatchedPublishingSubjects } from './publishing_batcher';
+export {
+  useBatchedOptionalPublishingSubjects,
+  useBatchedPublishingSubjects,
+} from './publishing_batcher';
 export { useStateFromPublishingSubject, usePublishingSubject } from './publishing_subject';
 export type {
   PublishingSubject,

diff --git a/packages/presentation/presentation_publishing/publishing_subject/publishing_batcher.ts b/packages/presentation/presentation_publishing/publishing_subject/publishing_batcher.ts
@@ -6,7 +6,7 @@
  * Side Public License, v 1.
  */
 
-import { useEffect, useMemo, useRef, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
 import { combineLatest, debounceTime, skip } from 'rxjs';
 import { AnyPublishingSubject, PublishingSubject, UnwrapPublishingSubjectTuple } from './types';
 
@@ -25,25 +25,28 @@ const hasSubjectsArrayChanged = (
 
 /**
  * Batches the latest values of multiple publishing subjects into a single object. Use this to avoid unnecessary re-renders.
- * You should avoid using this hook with subjects that your component pushes values to on user interaction, as it can cause a slight delay.
+ * Use when `subjects` may not be defined on initial component render.
+ *
  * @param subjects Publishing subjects array.
  *   When 'subjects' is expected to change, 'subjects' must be part of component react state.
  */
-export const useBatchedPublishingSubjects = <SubjectsType extends [...AnyPublishingSubject[]]>(
+export const useBatchedOptionalPublishingSubjects = <
+  SubjectsType extends [...AnyPublishingSubject[]]
+>(
   ...subjects: [...SubjectsType]
 ): UnwrapPublishingSubjectTuple<SubjectsType> => {
   const isFirstRender = useRef(true);
-  /**
-   * memoize and deep diff subjects to avoid rebuilding the subscription when the subjects are the same.
-   */
+
   const previousSubjects = useRef<SubjectsType>(subjects);
-  const subjectsToUse = useMemo(() => {
+  // Can not use 'useMemo' because 'subjects' gets a new reference on each call because of spread
+  const subjectsToUse = (() => {
+    // avoid rebuilding the subscription when the subjects are the same
     if (!hasSubjectsArrayChanged(previousSubjects.current ?? [], subjects)) {
       return previousSubjects.current;
     }
     previousSubjects.current = subjects;
     return subjects;
-  }, [subjects]);
+  })();
 
   /**
    * Set up latest published values state, initialized with the current values of the subjects.
@@ -94,6 +97,46 @@ export const useBatchedPublishingSubjects = <SubjectsType extends [...AnyPublish
   return latestPublishedValues;
 };
 
+/**
+ * Batches the latest values of multiple publishing subjects into a single object. Use this to avoid unnecessary re-renders.
+ * Use when `subjects` are static and do not change over the lifetime of the component.
+ *
+ * @param subjects Publishing subjects array.
+ */
+export const useBatchedPublishingSubjects = <
+  SubjectsType extends [...Array<PublishingSubject<any>>]
+>(
+  ...subjects: [...SubjectsType]
+): UnwrapPublishingSubjectTuple<SubjectsType> => {
+  /**
+   * Set up latest published values state, initialized with the current values of the subjects.
+   */
+  const [latestPublishedValues, setLatestPublishedValues] = useState<
+    UnwrapPublishingSubjectTuple<SubjectsType>
+  >(() => unwrapPublishingSubjectArray(subjects));
+
+  /**
+   * Subscribe to all subjects and update the latest values when any of them change.
+   */
+  useEffect(() => {
+    const subscription = combineLatest(subjects)
+      .pipe(
+        // When a new observer subscribes to a BehaviorSubject, it immediately receives the current value. Skip this emit.
+        skip(1),
+        debounceTime(0)
+      )
+      .subscribe((values) => {
+        setLatestPublishedValues(values as UnwrapPublishingSubjectTuple<SubjectsType>);
+      });
+    return () => subscription.unsubscribe();
+    // 'subjects' gets a new reference on each call because of spread
+    // Use 'useBatchedOptionalPublishingSubjects' when 'subjects' are expected to change.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);
+
+  return latestPublishedValues;
+};
+
 const unwrapPublishingSubjectArray = <T extends AnyPublishingSubject[]>(
   subjects: T
 ): UnwrapPublishingSubjectTuple<T> => {

diff --git a/packages/presentation/presentation_publishing/publishing_subject/publishing_subject.test.tsx b/packages/presentation/presentation_publishing/publishing_subject/publishing_subject.test.tsx
@@ -11,11 +11,14 @@ import { BehaviorSubject } from 'rxjs';
 import { render, screen, waitFor } from '@testing-library/react';
 import '@testing-library/jest-dom';
 import userEvent from '@testing-library/user-event';
-import { useBatchedPublishingSubjects } from './publishing_batcher';
+import {
+  useBatchedPublishingSubjects,
+  useBatchedOptionalPublishingSubjects,
+} from './publishing_batcher';
 import { useStateFromPublishingSubject } from './publishing_subject';
 import { PublishingSubject } from './types';
 
-describe('useBatchedPublishingSubjects', () => {
+describe('publishing subject', () => {
   describe('render', () => {
     let subject1: BehaviorSubject<number>;
     let subject2: BehaviorSubject<number>;
@@ -56,7 +59,6 @@ describe('useBatchedPublishingSubjects', () => {
           <>
             <button onClick={incrementAll} />
             <span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
-            <div data-test-subj="renderCount">{renderCount}</div>
           </>
         );
       }
@@ -72,7 +74,7 @@ describe('useBatchedPublishingSubjects', () => {
           screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
         ).toBeInTheDocument();
       });
-      expect(screen.getByTestId('renderCount')).toHaveTextContent('2');
+      expect(renderCount).toBe(2);
     });
 
     test('should batch state updates when using useBatchedPublishingSubjects', async () => {
@@ -97,7 +99,6 @@ describe('useBatchedPublishingSubjects', () => {
               }}
             />
             <span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
-            <div data-test-subj="renderCount">{renderCount}</div>
           </>
         );
       }
@@ -113,7 +114,48 @@ describe('useBatchedPublishingSubjects', () => {
           screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
         ).toBeInTheDocument();
       });
-      expect(screen.getByTestId('renderCount')).toHaveTextContent('2');
+      expect(renderCount).toBe(2);
+    });
+
+    test('should batch state updates when using useBatchedOptionalPublishingSubjects', async () => {
+      let renderCount = 0;
+      function Component() {
+        const [value1, value2, value3, value4, value5, value6] =
+          useBatchedOptionalPublishingSubjects(
+            subject1,
+            subject2,
+            subject3,
+            subject4,
+            subject5,
+            subject6
+          );
+
+        renderCount++;
+        return (
+          <>
+            <button
+              onClick={() => {
+                // using setTimeout to move next calls outside of callstack from onClick
+                setTimeout(incrementAll, 0);
+              }}
+            />
+            <span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
+          </>
+        );
+      }
+      render(<Component />);
+      await waitFor(() => {
+        expect(
+          screen.getByText('value1: 0, value2: 0, value3: 0, value4: 0, value5: 0, value6: 0')
+        ).toBeInTheDocument();
+      });
+      userEvent.click(screen.getByRole('button'));
+      await waitFor(() => {
+        expect(
+          screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
+        ).toBeInTheDocument();
+      });
+      expect(renderCount).toBe(2);
     });
 
     test('should render for each state update outside of click handler', async () => {
@@ -136,7 +178,6 @@ describe('useBatchedPublishingSubjects', () => {
               }}
             />
             <span>{`value1: ${value1}, value2: ${value2}, value3: ${value3}, value4: ${value4}, value5: ${value5}, value6: ${value6}`}</span>
-            <div data-test-subj="renderCount">{renderCount}</div>
           </>
         );
       }
@@ -152,33 +193,33 @@ describe('useBatchedPublishingSubjects', () => {
           screen.getByText('value1: 1, value2: 1, value3: 1, value4: 1, value5: 1, value6: 1')
         ).toBeInTheDocument();
       });
-      expect(screen.getByTestId('renderCount')).toHaveTextContent('7');
+      expect(renderCount).toBe(7);
     });
   });
 
   describe('Publishing subject is undefined on first render', () => {
-    test('useBatchedPublishingSubjects state should update when publishing subject is provided', async () => {
+    test('useBatchedOptionalPublishingSubjects should update state when publishing subject is provided', async () => {
       let renderCount = 0;
       function Component() {
         // When subjects is expected to change, subjects must be part of react state.
-        const [subjectFoo, setSubjectFoo] = useState<PublishingSubject<string> | undefined>(
+        const [subjectFoo, setSubjectFoo] = useState<BehaviorSubject<string> | undefined>(
           undefined
         );
-        const [valueFoo] = useBatchedPublishingSubjects(subjectFoo);
+        const [valueFoo] = useBatchedOptionalPublishingSubjects(subjectFoo);
 
         renderCount++;
         return (
           <>
             <button
               onClick={() => {
-                // using setTimeout to move next calls outside of callstack from onClick
-                setTimeout(() => {
-                  setSubjectFoo(new BehaviorSubject<string>('foo'));
-                }, 0);
+                if (!subjectFoo) {
+                  setSubjectFoo(new BehaviorSubject<string>('foo1'));
+                } else {
+                  subjectFoo.next('foo2');
+                }
               }}
             />
             <span>{`valueFoo: ${valueFoo}`}</span>
-            <div data-test-subj="renderCount">{renderCount}</div>
           </>
         );
       }
@@ -187,13 +228,14 @@ describe('useBatchedPublishingSubjects', () => {
         expect(screen.getByText('valueFoo: undefined')).toBeInTheDocument();
       });
       userEvent.click(screen.getByRole('button'));
+      userEvent.click(screen.getByRole('button'));
       await waitFor(() => {
-        expect(screen.getByText('valueFoo: foo')).toBeInTheDocument();
+        expect(screen.getByText('valueFoo: foo2')).toBeInTheDocument();
       });
-      expect(screen.getByTestId('renderCount')).toHaveTextContent('3');
+      expect(renderCount).toBe(4);
     });
 
-    test('useStateFromPublishingSubject state should update when publishing subject is provided', async () => {
+    test('useStateFromPublishingSubject should update state when publishing subject is provided', async () => {
       let renderCount = 0;
       function Component() {
         // When subject is expected to change, subject must be part of react state.
@@ -207,14 +249,10 @@ describe('useBatchedPublishingSubjects', () => {
           <>
             <button
               onClick={() => {
-                // using setTimeout to move next calls outside of callstack from onClick
-                setTimeout(() => {
-                  setSubjectFoo(new BehaviorSubject<string>('foo'));
-                }, 0);
+                setSubjectFoo(new BehaviorSubject<string>('foo'));
               }}
             />
             <span>{`valueFoo: ${valueFoo}`}</span>
-            <div data-test-subj="renderCount">{renderCount}</div>
           </>
         );
       }
@@ -226,7 +264,7 @@ describe('useBatchedPublishingSubjects', () => {
       await waitFor(() => {
         expect(screen.getByText('valueFoo: foo')).toBeInTheDocument();
       });
-      expect(screen.getByTestId('renderCount')).toHaveTextContent('3');
+      expect(renderCount).toBe(3);
     });
   });
 });
diff --git a/src/plugins/image_embeddable/public/components/image_embeddable.tsx b/src/plugins/image_embeddable/public/components/image_embeddable.tsx
@@ -10,6 +10,7 @@ import React, { useEffect, useState } from 'react';
 
 import { PublishingSubject, useBatchedPublishingSubjects } from '@kbn/presentation-publishing';
 
+import { BehaviorSubject } from 'rxjs';
 import { imageClickTrigger } from '../actions';
 import { ImageEmbeddableApi } from '../image_embeddable/types';
 import { FileImageMetadata, FilesClient, imageEmbeddableFileKind } from '../imports';
@@ -32,7 +33,7 @@ interface ImageEmbeddableProps {
 export const ImageEmbeddable = ({ api, filesClient }: ImageEmbeddableProps) => {
   const [imageConfig, dynamicActionsState] = useBatchedPublishingSubjects(
     api.imageConfig$,
-    api.dynamicActionsState$
+    api.dynamicActionsState$ ?? new BehaviorSubject<undefined>(undefined)
   );
   const [hasTriggerActions, setHasTriggerActions] = useState(false);
 

diff --git a/...resentation_panel/public/panel_component/panel_header/presentation_panel_context_menu.tsx b/...resentation_panel/public/panel_component/panel_header/presentation_panel_context_menu.tsx
@@ -21,7 +21,10 @@ import {
 } from '@elastic/eui';
 import { Action, buildContextMenuForActions } from '@kbn/ui-actions-plugin/public';
 
-import { getViewModeSubject, useBatchedPublishingSubjects } from '@kbn/presentation-publishing';
+import {
+  getViewModeSubject,
+  useBatchedOptionalPublishingSubjects,
+} from '@kbn/presentation-publishing';
 import { uiActions } from '../../kibana_services';
 import { contextMenuTrigger, CONTEXT_MENU_TRIGGER } from '../../panel_actions';
 import { getContextMenuAriaLabel } from '../presentation_panel_strings';
@@ -43,7 +46,7 @@ export const PresentationPanelContextMenu = ({
   const [isContextMenuOpen, setIsContextMenuOpen] = useState<boolean | undefined>(undefined);
   const [contextMenuPanels, setContextMenuPanels] = useState<EuiContextMenuPanelDescriptor[]>([]);
 
-  const [title, parentViewMode] = useBatchedPublishingSubjects(
+  const [title, parentViewMode] = useBatchedOptionalPublishingSubjects(
     api.panelTitle,
 
     /**

diff --git a/src/plugins/presentation_panel/public/panel_component/presentation_panel_internal.tsx b/src/plugins/presentation_panel/public/panel_component/presentation_panel_internal.tsx
@@ -12,7 +12,7 @@ import {
   apiPublishesPhaseEvents,
   apiHasParentApi,
   apiPublishesViewMode,
-  useBatchedPublishingSubjects,
+  useBatchedOptionalPublishingSubjects,
 } from '@kbn/presentation-publishing';
 import classNames from 'classnames';
 import React, { useEffect, useMemo, useState } from 'react';
@@ -57,7 +57,7 @@ export const PresentationPanelInternal = <
     defaultPanelTitle,
     rawViewMode,
     parentHidePanelTitle,
-  ] = useBatchedPublishingSubjects(
+  ] = useBatchedOptionalPublishingSubjects(
     api?.dataLoading,
     api?.blockingError,
     api?.panelTitle,