From f2978c7af8ec94fe40e60d2b30a6572a4d179e68 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Fri, 16 Feb 2024 13:12:18 -0500 Subject: [PATCH] refactor(app): Remove notification event emitters (#14504) Partially closes RAUT-990 --- app/src/redux/shell/remote.ts | 12 +- app/src/redux/shell/types.ts | 10 +- .../__tests__/useNotifyService.test.ts | 204 ++++++++++++++++++ app/src/resources/useNotifyService.ts | 17 +- 4 files changed, 221 insertions(+), 22 deletions(-) create mode 100644 app/src/resources/__tests__/useNotifyService.test.ts diff --git a/app/src/redux/shell/remote.ts b/app/src/redux/shell/remote.ts index 9671f600e5e..06270457867 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, + callback: (data: NotifyResponseData) => void +): void { remote.ipcRenderer.on( 'notify', (_, shellHostname, shellTopic, shellMessage) => { if (hostname === shellHostname && topic === shellTopic) { - eventEmitter.emit('data', shellMessage) + callback(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/__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() + }) +}) diff --git a/app/src/resources/useNotifyService.ts b/app/src/resources/useNotifyService.ts index 87398f4bc50..2b114cf5bc0 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,10 @@ export function useNotifyService({ hostname != null && staleTime !== Infinity ) { - const eventEmitter = appShellListener(hostname, topic) - eventEmitter.on('data', onDataListener) + appShellListener(hostname, topic, onDataEvent) dispatch(notifySubscribeAction(hostname, topic)) return () => { - eventEmitter.off('data', onDataListener) if (hostname != null) { dispatch(notifyUnsubscribeAction(hostname, topic)) } @@ -77,7 +66,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