From d0d4c4b320632cc6c5a610b6ec22d9305f81396b Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Thu, 15 Feb 2024 15:22:57 -0500 Subject: [PATCH 1/3] refactor(app): remove notification event emitters --- app/src/redux/shell/remote.ts | 12 +++++------- app/src/redux/shell/types.ts | 10 +++++++++- app/src/resources/useNotifyService.ts | 18 ++++-------------- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/app/src/redux/shell/remote.ts b/app/src/redux/shell/remote.ts index 9671f600e5e..093ae1f4b92 100644 --- a/app/src/redux/shell/remote.ts +++ b/app/src/redux/shell/remote.ts @@ -1,10 +1,9 @@ // access main process remote modules via attachments to `global` import assert from 'assert' -import { EventEmitter } from 'events' import type { AxiosRequestConfig } from 'axios' import type { ResponsePromise } from '@opentrons/api-client' -import type { Remote, NotifyTopic } from './types' +import type { Remote, NotifyTopic, NotifyResponseData } from './types' const emptyRemote: Remote = {} as any @@ -40,16 +39,15 @@ export function appShellRequestor( export function appShellListener( hostname: string | null, - topic: NotifyTopic -): EventEmitter { - const eventEmitter = new EventEmitter() + topic: NotifyTopic, + listenerCb: (data: NotifyResponseData) => void +): void { remote.ipcRenderer.on( 'notify', (_, shellHostname, shellTopic, shellMessage) => { if (hostname === shellHostname && topic === shellTopic) { - eventEmitter.emit('data', shellMessage) + listenerCb(shellMessage) } } ) - return eventEmitter } diff --git a/app/src/redux/shell/types.ts b/app/src/redux/shell/types.ts index 874b98296ef..e2baffba65f 100644 --- a/app/src/redux/shell/types.ts +++ b/app/src/redux/shell/types.ts @@ -12,13 +12,21 @@ export interface Remote { event: IpcMainEvent, hostname: string, topic: NotifyTopic, - message: string | Object, + message: NotifyResponseData | NotifyNetworkError, ...args: unknown[] ) => void ) => void } } +interface NotifyRefetchData { + refetchUsingHTTP: boolean + statusCode: never +} + +export type NotifyNetworkError = 'ECONNFAILED' | 'ECONNREFUSED' +export type NotifyResponseData = NotifyRefetchData | NotifyNetworkError + interface File { sha512: string url: string diff --git a/app/src/resources/useNotifyService.ts b/app/src/resources/useNotifyService.ts index 87398f4bc50..05a73337cae 100644 --- a/app/src/resources/useNotifyService.ts +++ b/app/src/resources/useNotifyService.ts @@ -12,22 +12,13 @@ import { } from '../redux/analytics' import type { UseQueryOptions } from 'react-query' -import type { NotifyTopic } from '../redux/shell/types' +import type { NotifyTopic, NotifyResponseData } from '../redux/shell/types' export interface QueryOptionsWithPolling extends UseQueryOptions { forceHttpPolling?: boolean } -interface NotifyRefetchData { - refetchUsingHTTP: boolean - statusCode: never -} - -export type NotifyNetworkError = 'ECONNFAILED' | 'ECONNREFUSED' - -type NotifyResponseData = NotifyRefetchData | NotifyNetworkError - interface UseNotifyServiceProps { topic: NotifyTopic refetchUsingHTTP: () => void @@ -55,12 +46,11 @@ export function useNotifyService({ hostname != null && staleTime !== Infinity ) { - const eventEmitter = appShellListener(hostname, topic) - eventEmitter.on('data', onDataListener) + const listenerCb = (data: NotifyResponseData): void => onDataEvent(data) + appShellListener(hostname, topic, listenerCb) dispatch(notifySubscribeAction(hostname, topic)) return () => { - eventEmitter.off('data', onDataListener) if (hostname != null) { dispatch(notifyUnsubscribeAction(hostname, topic)) } @@ -77,7 +67,7 @@ export function useNotifyService({ return { isNotifyError: isNotifyError.current } - function onDataListener(data: NotifyResponseData): void { + function onDataEvent(data: NotifyResponseData): void { if (!isNotifyError.current) { if (data === 'ECONNFAILED' || data === 'ECONNREFUSED') { isNotifyError.current = true From 099e1d3767c0e5a3f5d415c6a7e89435fce29539 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Fri, 16 Feb 2024 08:06:01 -0500 Subject: [PATCH 2/3] clean up callback --- app/src/redux/shell/remote.ts | 4 ++-- app/src/resources/useNotifyService.ts | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/src/redux/shell/remote.ts b/app/src/redux/shell/remote.ts index 093ae1f4b92..06270457867 100644 --- a/app/src/redux/shell/remote.ts +++ b/app/src/redux/shell/remote.ts @@ -40,13 +40,13 @@ export function appShellRequestor( export function appShellListener( hostname: string | null, topic: NotifyTopic, - listenerCb: (data: NotifyResponseData) => void + callback: (data: NotifyResponseData) => void ): void { remote.ipcRenderer.on( 'notify', (_, shellHostname, shellTopic, shellMessage) => { if (hostname === shellHostname && topic === shellTopic) { - listenerCb(shellMessage) + callback(shellMessage) } } ) diff --git a/app/src/resources/useNotifyService.ts b/app/src/resources/useNotifyService.ts index 05a73337cae..2b114cf5bc0 100644 --- a/app/src/resources/useNotifyService.ts +++ b/app/src/resources/useNotifyService.ts @@ -46,8 +46,7 @@ export function useNotifyService({ hostname != null && staleTime !== Infinity ) { - const listenerCb = (data: NotifyResponseData): void => onDataEvent(data) - appShellListener(hostname, topic, listenerCb) + appShellListener(hostname, topic, onDataEvent) dispatch(notifySubscribeAction(hostname, topic)) return () => { From f3a2bb3a458925bea2bdb84a0ca9d969f9a2dc99 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Fri, 16 Feb 2024 12:58:35 -0500 Subject: [PATCH 3/3] test --- .../__tests__/useNotifyService.test.ts | 204 ++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 app/src/resources/__tests__/useNotifyService.test.ts diff --git a/app/src/resources/__tests__/useNotifyService.test.ts b/app/src/resources/__tests__/useNotifyService.test.ts new file mode 100644 index 00000000000..6d07f4e23f4 --- /dev/null +++ b/app/src/resources/__tests__/useNotifyService.test.ts @@ -0,0 +1,204 @@ +import { useDispatch } from 'react-redux' +import { renderHook } from '@testing-library/react' + +import { useHost } from '@opentrons/react-api-client' + +import { useNotifyService } from '../useNotifyService' +import { appShellListener } from '../../redux/shell/remote' +import { useTrackEvent } from '../../redux/analytics' +import { + notifySubscribeAction, + notifyUnsubscribeAction, +} from '../../redux/shell' + +import type { HostConfig } from '@opentrons/api-client' +import type { QueryOptionsWithPolling } from '../useNotifyService' + +jest.mock('react-redux') +jest.mock('@opentrons/react-api-client') +jest.mock('../../redux/analytics') +jest.mock('../../redux/shell/remote', () => ({ + appShellListener: jest.fn(), +})) + +const MOCK_HOST_CONFIG: HostConfig = { hostname: 'MOCK_HOST' } +const MOCK_TOPIC = '/test/topic' as any +const MOCK_OPTIONS: QueryOptionsWithPolling = { + forceHttpPolling: false, +} + +const mockUseHost = useHost as jest.MockedFunction +const mockUseDispatch = useDispatch as jest.MockedFunction +const mockUseTrackEvent = useTrackEvent as jest.MockedFunction< + typeof useTrackEvent +> +const mockAppShellListener = appShellListener as jest.MockedFunction< + typeof appShellListener +> + +describe('useNotifyService', () => { + let mockDispatch: jest.Mock + let mockTrackEvent: jest.Mock + let mockHTTPRefetch: jest.Mock + + beforeEach(() => { + mockDispatch = jest.fn() + mockHTTPRefetch = jest.fn() + mockTrackEvent = jest.fn() + mockUseTrackEvent.mockReturnValue(mockTrackEvent) + mockUseDispatch.mockReturnValue(mockDispatch) + mockUseHost.mockReturnValue(MOCK_HOST_CONFIG) + }) + + afterEach(() => { + mockUseDispatch.mockClear() + jest.clearAllMocks() + }) + + it('should trigger an HTTP refetch and subscribe action on initial mount', () => { + renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + expect(mockHTTPRefetch).toHaveBeenCalled() + expect(mockDispatch).toHaveBeenCalledWith( + notifySubscribeAction(MOCK_HOST_CONFIG.hostname, MOCK_TOPIC) + ) + expect(mockDispatch).not.toHaveBeenCalledWith( + notifyUnsubscribeAction(MOCK_HOST_CONFIG.hostname, MOCK_TOPIC) + ) + expect(appShellListener).toHaveBeenCalled() + }) + + it('should trigger an unsubscribe action on dismount', () => { + const { unmount } = renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + unmount() + expect(mockDispatch).toHaveBeenCalledWith( + notifyUnsubscribeAction(MOCK_HOST_CONFIG.hostname, MOCK_TOPIC) + ) + }) + + it('should return no notify error if there was a successful topic subscription', () => { + const { result } = renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + expect(result.current.isNotifyError).toBe(false) + }) + + it('should not subscribe to notifications if forceHttpPolling is true', () => { + renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: { ...MOCK_OPTIONS, forceHttpPolling: true }, + } as any) + ) + expect(mockHTTPRefetch).toHaveBeenCalled() + expect(appShellListener).not.toHaveBeenCalled() + expect(mockDispatch).not.toHaveBeenCalled() + }) + + it('should not subscribe to notifications if enabled is false', () => { + renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: { ...MOCK_OPTIONS, enabled: false }, + } as any) + ) + expect(mockHTTPRefetch).toHaveBeenCalled() + expect(appShellListener).not.toHaveBeenCalled() + expect(mockDispatch).not.toHaveBeenCalled() + }) + + it('should not subscribe to notifications if staleTime is Infinity', () => { + renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: { ...MOCK_OPTIONS, staleTime: Infinity }, + } as any) + ) + expect(mockHTTPRefetch).toHaveBeenCalled() + expect(appShellListener).not.toHaveBeenCalled() + expect(mockDispatch).not.toHaveBeenCalled() + }) + + it('should log an error if hostname is null', () => { + mockUseHost.mockReturnValue({ hostname: null } as any) + const errorSpy = jest.spyOn(console, 'error') + errorSpy.mockImplementation(() => {}) + + renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + expect(errorSpy).toHaveBeenCalledWith( + 'NotifyService expected hostname, received null for topic:', + MOCK_TOPIC + ) + errorSpy.mockRestore() + }) + + it('should return a notify error and fire an analytics reporting event if the connection was refused', () => { + mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => { + mockCb('ECONNREFUSED') + }) + const { result, rerender } = renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + expect(mockTrackEvent).toHaveBeenCalled() + rerender() + expect(result.current.isNotifyError).toBe(true) + }) + + it('should return a notify error if the connection failed', () => { + mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => { + mockCb('ECONNFAILED') + }) + const { result, rerender } = renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + rerender() + expect(result.current.isNotifyError).toBe(true) + }) + + it('should trigger an HTTP refetch if the refetch flag was returned', () => { + mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => { + mockCb({ refetchUsingHTTP: true }) + }) + const { rerender } = renderHook(() => + useNotifyService({ + topic: MOCK_TOPIC, + refetchUsingHTTP: mockHTTPRefetch, + options: MOCK_OPTIONS, + } as any) + ) + rerender() + expect(mockHTTPRefetch).toHaveBeenCalled() + }) +})