From 1f94fd4e53cfdc9a8e74fd0230e00d218f0d5062 Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Fri, 2 Feb 2024 13:30:29 -0500 Subject: [PATCH] review feedback --- .../eui_markdown_react_embeddable.tsx | 2 +- .../react_embeddable_api.test.tsx | 4 ++-- .../react_embeddable_api.ts | 2 +- .../react_embeddable_registry.test.tsx | 1 - .../react_embeddable_registry.ts | 7 +++++++ .../react_embeddable_renderer.test.tsx | 6 ++++-- .../react_embeddable_unsaved_changes.ts | 17 +++++++++-------- 7 files changed, 24 insertions(+), 15 deletions(-) diff --git a/examples/embeddable_examples/public/react_embeddables/eui_markdown_react_embeddable.tsx b/examples/embeddable_examples/public/react_embeddables/eui_markdown_react_embeddable.tsx index 12ff63aeeba66..b525181f307a9 100644 --- a/examples/embeddable_examples/public/react_embeddables/eui_markdown_react_embeddable.tsx +++ b/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, diff --git a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.test.tsx b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.test.tsx index 1a281eb4ef785..4ec729119d87e 100644 --- a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.test.tsx +++ b/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 () => { diff --git a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.ts b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.ts index 74ac3ad61378d..896a9fb05468b 100644 --- a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_api.ts +++ b/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(); diff --git a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.test.tsx b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.test.tsx index 507d0e91f2c11..11d7a58fa0890 100644 --- a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.test.tsx +++ b/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); }); diff --git a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.ts b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.ts index 768939619db64..2f014b84c07ab 100644 --- a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.ts +++ b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_registry.ts @@ -24,6 +24,13 @@ export const registerReactEmbeddableFactory = < key: string, factory: ReactEmbeddableFactory ) => { + 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; }; diff --git a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.test.tsx b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.test.tsx index 01357f56b5362..376923de108d1 100644 --- a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.test.tsx +++ b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_renderer.test.tsx @@ -20,8 +20,11 @@ describe('react embeddable renderer', () => { }), }; - it('deserializes given state', () => { + beforeAll(() => { registerReactEmbeddableFactory('test', testEmbeddableFactory); + }); + + it('deserializes given state', () => { render(); expect(testEmbeddableFactory.deserializeState).toHaveBeenCalledWith({ rawState: { blorp: 'blorp?' }, @@ -29,7 +32,6 @@ describe('react embeddable renderer', () => { }); it('renders the given component once it resolves', () => { - registerReactEmbeddableFactory('test', testEmbeddableFactory); render(); waitFor(() => { expect(screen.findByText('SUPER TEST COMPONENT')).toBeInTheDocument(); diff --git a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_unsaved_changes.ts b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_unsaved_changes.ts index ca299a91cd49c..1e760b55f748a 100644 --- a/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_unsaved_changes.ts +++ b/src/plugins/embeddable/public/react_embeddable_system/react_embeddable_unsaved_changes.ts @@ -22,7 +22,8 @@ const getInitialValuesFromComparators = ( ) => { const initialValues: Partial = {}; 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 = ( } const latestChanges: Partial = {}; 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 = ( @@ -65,7 +64,8 @@ export const useReactEmbeddableUnsavedChanges = > = []; const keys: Array = []; for (const key of Object.keys(comparators) as Array) { - subjects.push(comparators[key][0] as PublishingSubject); + const comparatorSubject = comparators[key][0]; // 0th element of tuple is the subject + subjects.push(comparatorSubject as PublishingSubject); keys.push(key); } return { comparatorKeys: keys, comparatorSubjects: subjects }; @@ -121,7 +121,8 @@ export const useReactEmbeddableUnsavedChanges = { 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