review feedback

examples/embeddable_examples/public/react_embeddables/eui_markdown_react_embeddable.tsx

@@ -87,7 +87,7 @@ const markdownEmbeddableFactory: ReactEmbeddableFactory<
        * Publish the API. This is what gets forwarded to the Actions framework, and to whatever the
        * parent of this embeddable is.
        */
-      const { thisApi } = useReactEmbeddableApiHandle(
+      const thisApi = useReactEmbeddableApiHandle(
         {
           ...titlesApi,
           unsavedChanges,

src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.test.tsx

@@ -40,8 +40,8 @@ describe('react embeddable api', () => {
       )
     );
 
-    expect(result.current.thisApi.bork()).toEqual('bork');
-    expect(result.current.thisApi.serializeState()).toEqual({ bork: 'borkbork' });
+    expect(result.current.bork()).toEqual('bork');
+    expect(result.current.serializeState()).toEqual({ bork: 'borkbork' });
   });
 
   it('publishes the API into the provided ref', async () => {

src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.ts

@@ -50,7 +50,7 @@ export const useReactEmbeddableApiHandle = <
   // eslint-disable-next-line react-hooks/exhaustive-deps
   useImperativeHandle(ref, () => thisApi, [uuid]);
 
-  return { thisApi, parentApi };
+  return thisApi;
 };
 
 export const initializeReactEmbeddableUuid = (maybeId?: string) => maybeId ?? generateId();

src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.test.tsx

@@ -31,7 +31,6 @@ describe('react embeddable registry', () => {
   });
 
   it('can check if a factory is registered', () => {
-    registerReactEmbeddableFactory('test', testEmbeddableFactory);
     expect(reactEmbeddableRegistryHasKey('test')).toBe(true);
     expect(reactEmbeddableRegistryHasKey('notRegistered')).toBe(false);
   });

src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.ts

@@ -24,6 +24,13 @@ export const registerReactEmbeddableFactory = <
   key: string,
   factory: ReactEmbeddableFactory<StateType, APIType>
 ) => {
+  if (registry[key] !== undefined)
+    throw new Error(
+      i18n.translate('embeddableApi.reactEmbeddable.factoryAlreadyExistsError', {
+        defaultMessage: 'An embeddable factory for for type: {key} is already registered.',
+        values: { key },
+      })
+    );
   registry[key] = factory;
 };
 

src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.test.tsx

@@ -20,16 +20,18 @@ describe('react embeddable renderer', () => {
     }),
   };
 
-  it('deserializes given state', () => {
+  beforeAll(() => {
     registerReactEmbeddableFactory('test', testEmbeddableFactory);
+  });
+
+  it('deserializes given state', () => {
     render(<ReactEmbeddableRenderer type={'test'} state={{ rawState: { blorp: 'blorp?' } }} />);
     expect(testEmbeddableFactory.deserializeState).toHaveBeenCalledWith({
       rawState: { blorp: 'blorp?' },
     });
   });
 
   it('renders the given component once it resolves', () => {
-    registerReactEmbeddableFactory('test', testEmbeddableFactory);
     render(<ReactEmbeddableRenderer type={'test'} state={{ rawState: { blorp: 'blorp?' } }} />);
     waitFor(() => {
       expect(screen.findByText('SUPER TEST COMPONENT')).toBeInTheDocument();

src/plugins/embeddable/public/react_embeddable_system/react_embeddable_unsaved_changes.ts

@@ -22,7 +22,8 @@ const getInitialValuesFromComparators = <StateType extends object = object>(
 ) => {
   const initialValues: Partial<StateType> = {};
   for (const key of comparatorKeys) {
-    initialValues[key] = comparators[key][0]?.value;
+    const comparatorSubject = comparators[key][0]; // 0th element of tuple is the subject
+    initialValues[key] = comparatorSubject?.value;
   }
   return initialValues;
 };
@@ -39,15 +40,13 @@ const runComparators = <StateType extends object = object>(
   }
   const latestChanges: Partial<StateType> = {};
   for (const key of comparatorKeys) {
-    const comparator = comparators[key]?.[2] ?? defaultComparator;
+    const customComparator = comparators[key]?.[2]; // 2nd element of the tuple is the custom comparator
+    const comparator = customComparator ?? defaultComparator;
     if (!comparator(lastSavedState?.[key], latestState[key], lastSavedState, latestState)) {
       latestChanges[key] = latestState[key];
     }
   }
-  if (Object.keys(latestChanges).length > 0) {
-    return latestChanges;
-  }
-  return;
+  return Object.keys(latestChanges).length > 0 ? latestChanges : undefined;
 };
 
 export const useReactEmbeddableUnsavedChanges = <StateType extends object = object>(
@@ -65,7 +64,8 @@ export const useReactEmbeddableUnsavedChanges = <StateType extends object = obje
     const subjects: Array<PublishingSubject<unknown>> = [];
     const keys: Array<keyof StateType> = [];
     for (const key of Object.keys(comparators) as Array<keyof StateType>) {
-      subjects.push(comparators[key][0] as PublishingSubject<unknown>);
+      const comparatorSubject = comparators[key][0]; // 0th element of tuple is the subject
+      subjects.push(comparatorSubject as PublishingSubject<unknown>);
       keys.push(key);
     }
     return { comparatorKeys: keys, comparatorSubjects: subjects };
@@ -121,7 +121,8 @@ export const useReactEmbeddableUnsavedChanges = <StateType extends object = obje
   const resetUnsavedChanges = useCallback(() => {
     const lastSaved = lastSavedStateSubject?.getValue();
     for (const key of comparatorKeys) {
-      comparators[key][1](lastSaved?.[key] as StateType[typeof key]);
+      const setter = comparators[key][1]; // setter function is the 1st element of the tuple
+      setter(lastSaved?.[key] as StateType[typeof key]);
     }
 
     // disable exhaustive deps because the comparators must be static