From ae38ff19ae37a94b020cb0978f4fddb6c8b032d8 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Thu, 29 Feb 2024 17:21:11 -0500 Subject: [PATCH] refactor(app): Clean up notify hooks (#14562) Partially closes RAUT-990 --- app-shell-odd/src/notify.ts | 19 ++--- app-shell/src/notify.ts | 19 ++--- .../__tests__/useNotifyService.test.ts | 66 +++++------------ .../useNotifyCurrentMaintenanceRun.ts | 40 +++++------ .../resources/runs/useNotifyAllRunsQuery.ts | 32 ++++----- .../runs/useNotifyLastRunCommandKey.ts | 29 ++++---- app/src/resources/runs/useNotifyRunQuery.ts | 36 +++++----- app/src/resources/useNotifyService.ts | 71 ++++++++----------- 8 files changed, 136 insertions(+), 176 deletions(-) diff --git a/app-shell-odd/src/notify.ts b/app-shell-odd/src/notify.ts index 7ff2f73b22f..89993ebec20 100644 --- a/app-shell-odd/src/notify.ts +++ b/app-shell-odd/src/notify.ts @@ -276,16 +276,19 @@ function handleDecrementSubscriptionCount( hostname: string, topic: NotifyTopic ): void { - const { client, subscriptions } = connectionStore[hostname] - if (topic in subscriptions) { - subscriptions[topic] -= 1 - if (subscriptions[topic] <= 0) { - delete subscriptions[topic] + const host = connectionStore[hostname] + if (host) { + const { client, subscriptions } = host + if (topic in subscriptions) { + subscriptions[topic] -= 1 + if (subscriptions[topic] <= 0) { + delete subscriptions[topic] + } } - } - if (Object.keys(subscriptions).length <= 0) { - client?.end() + if (Object.keys(subscriptions).length <= 0) { + client?.end() + } } } diff --git a/app-shell/src/notify.ts b/app-shell/src/notify.ts index 9ec4c04e62b..d1f806feb3f 100644 --- a/app-shell/src/notify.ts +++ b/app-shell/src/notify.ts @@ -272,16 +272,19 @@ function handleDecrementSubscriptionCount( hostname: string, topic: NotifyTopic ): void { - const { client, subscriptions } = connectionStore[hostname] - if (topic in subscriptions) { - subscriptions[topic] -= 1 - if (subscriptions[topic] <= 0) { - delete subscriptions[topic] + const host = connectionStore[hostname] + if (host) { + const { client, subscriptions } = host + if (topic in subscriptions) { + subscriptions[topic] -= 1 + if (subscriptions[topic] <= 0) { + delete subscriptions[topic] + } } - } - if (Object.keys(subscriptions).length <= 0) { - client?.end() + if (Object.keys(subscriptions).length <= 0) { + client?.end() + } } } diff --git a/app/src/resources/__tests__/useNotifyService.test.ts b/app/src/resources/__tests__/useNotifyService.test.ts index bb1133e40e3..6946f8f8c17 100644 --- a/app/src/resources/__tests__/useNotifyService.test.ts +++ b/app/src/resources/__tests__/useNotifyService.test.ts @@ -59,15 +59,15 @@ describe('useNotifyService', () => { jest.clearAllMocks() }) - it('should trigger an HTTP refetch and subscribe action on initial mount', () => { + it('should trigger an HTTP refetch and subscribe action on a successful initial mount', () => { renderHook(() => useNotifyService({ topic: MOCK_TOPIC, - refetchUsingHTTP: mockHTTPRefetch, + setRefetchUsingHTTP: mockHTTPRefetch, options: MOCK_OPTIONS, } as any) ) - expect(mockHTTPRefetch).toHaveBeenCalled() + expect(mockHTTPRefetch).toHaveBeenCalledWith('once') expect(mockDispatch).toHaveBeenCalledWith( notifySubscribeAction(MOCK_HOST_CONFIG.hostname, MOCK_TOPIC) ) @@ -81,7 +81,7 @@ describe('useNotifyService', () => { const { unmount } = renderHook(() => useNotifyService({ topic: MOCK_TOPIC, - refetchUsingHTTP: mockHTTPRefetch, + setRefetchUsingHTTP: mockHTTPRefetch, options: MOCK_OPTIONS, } as any) ) @@ -91,22 +91,11 @@ describe('useNotifyService', () => { ) }) - 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, + setRefetchUsingHTTP: mockHTTPRefetch, options: { ...MOCK_OPTIONS, forceHttpPolling: true }, } as any) ) @@ -119,7 +108,7 @@ describe('useNotifyService', () => { renderHook(() => useNotifyService({ topic: MOCK_TOPIC, - refetchUsingHTTP: mockHTTPRefetch, + setRefetchUsingHTTP: mockHTTPRefetch, options: { ...MOCK_OPTIONS, enabled: false }, } as any) ) @@ -132,7 +121,7 @@ describe('useNotifyService', () => { renderHook(() => useNotifyService({ topic: MOCK_TOPIC, - refetchUsingHTTP: mockHTTPRefetch, + setRefetchUsingHTTP: mockHTTPRefetch, options: { ...MOCK_OPTIONS, staleTime: Infinity }, } as any) ) @@ -141,68 +130,47 @@ describe('useNotifyService', () => { expect(mockDispatch).not.toHaveBeenCalled() }) - it('should log an error if hostname is null', () => { + it('should set HTTP refetch to always if there is an error', () => { mockUseHost.mockReturnValue({ hostname: null } as any) - const errorSpy = jest.spyOn(console, 'error') - errorSpy.mockImplementation(() => {}) renderHook(() => useNotifyService({ topic: MOCK_TOPIC, - refetchUsingHTTP: mockHTTPRefetch, + setRefetchUsingHTTP: mockHTTPRefetch, options: MOCK_OPTIONS, } as any) ) - expect(errorSpy).toHaveBeenCalledWith( - 'NotifyService expected hostname, received null for topic:', - MOCK_TOPIC - ) - errorSpy.mockRestore() + expect(mockHTTPRefetch).toHaveBeenCalledWith('always') }) - it('should return a notify error and fire an analytics reporting event if the connection was refused', () => { + it('should return set HTTP refetch to always and fire an analytics reporting event if the connection was refused', () => { mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => { mockCb('ECONNREFUSED') }) - const { result, rerender } = renderHook(() => + const { rerender } = renderHook(() => useNotifyService({ topic: MOCK_TOPIC, - refetchUsingHTTP: mockHTTPRefetch, + setRefetchUsingHTTP: mockHTTPRefetch, options: MOCK_OPTIONS, } as any) ) expect(mockTrackEvent).toHaveBeenCalled() rerender() - expect(result.current.isNotifyError).toBe(true) + expect(mockHTTPRefetch).toHaveBeenCalledWith('always') }) - 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', () => { + it('should trigger a single 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, + setRefetchUsingHTTP: mockHTTPRefetch, options: MOCK_OPTIONS, } as any) ) rerender() - expect(mockHTTPRefetch).toHaveBeenCalled() + expect(mockHTTPRefetch).toHaveBeenCalledWith('once') }) }) diff --git a/app/src/resources/maintenance_runs/useNotifyCurrentMaintenanceRun.ts b/app/src/resources/maintenance_runs/useNotifyCurrentMaintenanceRun.ts index 4c634225958..2692e032d6a 100644 --- a/app/src/resources/maintenance_runs/useNotifyCurrentMaintenanceRun.ts +++ b/app/src/resources/maintenance_runs/useNotifyCurrentMaintenanceRun.ts @@ -1,41 +1,37 @@ import * as React from 'react' -import { useHost, useCurrentMaintenanceRun } from '@opentrons/react-api-client' +import { useCurrentMaintenanceRun } from '@opentrons/react-api-client' import { useNotifyService } from '../useNotifyService' import type { UseQueryResult } from 'react-query' import type { MaintenanceRun } from '@opentrons/api-client' -import type { QueryOptionsWithPolling } from '../useNotifyService' +import type { + QueryOptionsWithPolling, + HTTPRefetchFrequency, +} from '../useNotifyService' export function useNotifyCurrentMaintenanceRun( - options?: QueryOptionsWithPolling + options: QueryOptionsWithPolling = {} ): UseQueryResult | UseQueryResult { - const host = useHost() - const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false) + const [ + refetchUsingHTTP, + setRefetchUsingHTTP, + ] = React.useState(null) - const { isNotifyError } = useNotifyService({ + useNotifyService({ topic: 'robot-server/maintenance_runs/current_run', - refetchUsingHTTP: () => setRefetchUsingHTTP(true), - options: { - ...options, - enabled: host !== null && options?.enabled !== false, - }, + setRefetchUsingHTTP, + options, }) - const isNotifyEnabled = - !isNotifyError && - !options?.forceHttpPolling && - options?.staleTime !== Infinity - - if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true) - const isHTTPEnabled = - host !== null && options?.enabled !== false && refetchUsingHTTP - const httpQueryResult = useCurrentMaintenanceRun({ ...options, - enabled: isHTTPEnabled, - onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined, + enabled: options?.enabled !== false && refetchUsingHTTP != null, + onSettled: + refetchUsingHTTP === 'once' + ? () => setRefetchUsingHTTP(null) + : () => null, }) return httpQueryResult diff --git a/app/src/resources/runs/useNotifyAllRunsQuery.ts b/app/src/resources/runs/useNotifyAllRunsQuery.ts index 4fd9ec534a9..690d7a4ac11 100644 --- a/app/src/resources/runs/useNotifyAllRunsQuery.ts +++ b/app/src/resources/runs/useNotifyAllRunsQuery.ts @@ -8,39 +8,37 @@ import type { UseQueryResult } from 'react-query' import type { AxiosError } from 'axios' import type { HostConfig, GetRunsParams, Runs } from '@opentrons/api-client' import type { UseAllRunsQueryOptions } from '@opentrons/react-api-client/src/runs/useAllRunsQuery' -import type { QueryOptionsWithPolling } from '../useNotifyService' +import type { + QueryOptionsWithPolling, + HTTPRefetchFrequency, +} from '../useNotifyService' export function useNotifyAllRunsQuery( params: GetRunsParams = {}, options: QueryOptionsWithPolling = {}, hostOverride?: HostConfig | null ): UseQueryResult { - const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false) + const [ + refetchUsingHTTP, + setRefetchUsingHTTP, + ] = React.useState(null) - const { isNotifyError } = useNotifyService< - UseAllRunsQueryOptions, - AxiosError - >({ + useNotifyService({ topic: 'robot-server/runs', - refetchUsingHTTP: () => setRefetchUsingHTTP(true), + setRefetchUsingHTTP, options, hostOverride, }) - const isNotifyEnabled = - !isNotifyError && - !options?.forceHttpPolling && - options?.staleTime !== Infinity - - if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true) - const isHTTPEnabled = options?.enabled !== false && refetchUsingHTTP - const httpResponse = useAllRunsQuery( params, { ...(options as UseAllRunsQueryOptions), - enabled: isHTTPEnabled, - onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined, + enabled: options?.enabled !== false && refetchUsingHTTP != null, + onSettled: + refetchUsingHTTP === 'once' + ? () => setRefetchUsingHTTP(null) + : () => null, }, hostOverride ) diff --git a/app/src/resources/runs/useNotifyLastRunCommandKey.ts b/app/src/resources/runs/useNotifyLastRunCommandKey.ts index cf7a0ec650f..8600c4d66b6 100644 --- a/app/src/resources/runs/useNotifyLastRunCommandKey.ts +++ b/app/src/resources/runs/useNotifyLastRunCommandKey.ts @@ -4,28 +4,33 @@ import { useNotifyService } from '../useNotifyService' import { useLastRunCommandKey } from '../../organisms/Devices/hooks/useLastRunCommandKey' import type { CommandsData } from '@opentrons/api-client' -import type { QueryOptionsWithPolling } from '../useNotifyService' +import type { + QueryOptionsWithPolling, + HTTPRefetchFrequency, +} from '../useNotifyService' export function useNotifyLastRunCommandKey( runId: string, - options?: QueryOptionsWithPolling + options: QueryOptionsWithPolling = {} ): string | null { - const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false) + const [ + refetchUsingHTTP, + setRefetchUsingHTTP, + ] = React.useState(null) - const { isNotifyError } = useNotifyService({ + useNotifyService({ topic: 'robot-server/runs/current_command', - refetchUsingHTTP: () => setRefetchUsingHTTP(true), - options: options != null ? options : {}, + setRefetchUsingHTTP, + options, }) - const isNotifyEnabled = !isNotifyError && !options?.forceHttpPolling - if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true) - const isHTTPEnabled = options?.enabled !== false && refetchUsingHTTP - const httpResponse = useLastRunCommandKey(runId, { ...options, - enabled: isHTTPEnabled, - onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined, + enabled: options?.enabled !== false && refetchUsingHTTP != null, + onSettled: + refetchUsingHTTP === 'once' + ? () => setRefetchUsingHTTP(null) + : () => null, }) return httpResponse diff --git a/app/src/resources/runs/useNotifyRunQuery.ts b/app/src/resources/runs/useNotifyRunQuery.ts index fb7b5442bbd..1da90ee7a08 100644 --- a/app/src/resources/runs/useNotifyRunQuery.ts +++ b/app/src/resources/runs/useNotifyRunQuery.ts @@ -1,43 +1,39 @@ import * as React from 'react' -import { useRunQuery, useHost } from '@opentrons/react-api-client' +import { useRunQuery } from '@opentrons/react-api-client' import { useNotifyService } from '../useNotifyService' import type { UseQueryResult } from 'react-query' import type { Run } from '@opentrons/api-client' -import type { QueryOptionsWithPolling } from '../useNotifyService' +import type { + QueryOptionsWithPolling, + HTTPRefetchFrequency, +} from '../useNotifyService' import type { NotifyTopic } from '../../redux/shell/types' export function useNotifyRunQuery( runId: string | null, options: QueryOptionsWithPolling = {} ): UseQueryResult { - const host = useHost() - const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false) + const [ + refetchUsingHTTP, + setRefetchUsingHTTP, + ] = React.useState(null) - const { isNotifyError } = useNotifyService({ + useNotifyService({ topic: `robot-server/runs/${runId}` as NotifyTopic, - refetchUsingHTTP: () => setRefetchUsingHTTP(true), + setRefetchUsingHTTP, options: { ...options, enabled: options.enabled && runId != null }, }) - const isNotifyEnabled = - !isNotifyError && - !options?.forceHttpPolling && - options?.staleTime !== Infinity - - if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true) - const isHTTPEnabled = - options?.enabled !== false && - refetchUsingHTTP && - host !== null && - runId != null - const httpResponse = useRunQuery(runId, { ...options, - enabled: isHTTPEnabled, - onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined, + enabled: options?.enabled !== false && refetchUsingHTTP != null, + onSettled: + refetchUsingHTTP === 'once' + ? () => setRefetchUsingHTTP(null) + : () => null, }) return httpResponse diff --git a/app/src/resources/useNotifyService.ts b/app/src/resources/useNotifyService.ts index e56a680d720..3accf0b8082 100644 --- a/app/src/resources/useNotifyService.ts +++ b/app/src/resources/useNotifyService.ts @@ -16,6 +16,8 @@ import type { UseQueryOptions } from 'react-query' import type { HostConfig } from '@opentrons/api-client' import type { NotifyTopic, NotifyResponseData } from '../redux/shell/types' +export type HTTPRefetchFrequency = 'once' | 'always' | null + export interface QueryOptionsWithPolling extends UseQueryOptions { forceHttpPolling?: boolean @@ -23,71 +25,60 @@ export interface QueryOptionsWithPolling interface UseNotifyServiceProps { topic: NotifyTopic - refetchUsingHTTP: () => void + setRefetchUsingHTTP: (refetch: HTTPRefetchFrequency) => void options: QueryOptionsWithPolling hostOverride?: HostConfig | null } export function useNotifyService({ topic, - refetchUsingHTTP, + setRefetchUsingHTTP, options, hostOverride, -}: UseNotifyServiceProps): { isNotifyError: boolean } { +}: UseNotifyServiceProps): void { const dispatch = useDispatch() const hostFromProvider = useHost() const host = hostOverride ?? hostFromProvider const hostname = host?.hostname ?? null const doTrackEvent = useTrackEvent() const isFlex = useIsFlex(host?.robotName ?? '') - const isNotifyError = React.useRef(false) + const hasUsedNotifyService = React.useRef(false) const { enabled, staleTime, forceHttpPolling } = options + const shouldUseNotifications = + !forceHttpPolling && + enabled !== false && + hostname != null && + staleTime !== Infinity + React.useEffect(() => { - // Always fetch on initial mount. - refetchUsingHTTP() - if ( - !forceHttpPolling && - enabled !== false && - hostname != null && - staleTime !== Infinity - ) { + if (shouldUseNotifications) { + // Always fetch on initial mount. + setRefetchUsingHTTP('once') appShellListener(hostname, topic, onDataEvent) dispatch(notifySubscribeAction(hostname, topic)) + hasUsedNotifyService.current = true + } else setRefetchUsingHTTP('always') - return () => { - if (hostname != null) { - dispatch(notifyUnsubscribeAction(hostname, topic)) - } - } - } else { - if (hostname == null) { - console.error( - 'NotifyService expected hostname, received null for topic:', - topic - ) + return () => { + if (hasUsedNotifyService.current && hostname != null) { + dispatch(notifyUnsubscribeAction(hostname, topic)) } } - }, [topic, host]) - - return { isNotifyError: isNotifyError.current } + }, [topic, host, shouldUseNotifications]) function onDataEvent(data: NotifyResponseData): void { - if (!isNotifyError.current) { - if (data === 'ECONNFAILED' || data === 'ECONNREFUSED') { - isNotifyError.current = true - // TODO(jh 2023-02-23): remove the robot type check once OT-2s support MQTT. - if (data === 'ECONNREFUSED' && isFlex) { - doTrackEvent({ - name: ANALYTICS_NOTIFICATION_PORT_BLOCK_ERROR, - properties: {}, - }) - } - } else if ('refetchUsingHTTP' in data) { - refetchUsingHTTP() - } else { - console.log('Unexpected data received from notify service.') + if (data === 'ECONNFAILED' || data === 'ECONNREFUSED') { + setRefetchUsingHTTP('always') + // TODO(jh 2023-02-23): remove the robot type check once OT-2s support MQTT. + if (data === 'ECONNREFUSED' && isFlex) { + doTrackEvent({ + name: ANALYTICS_NOTIFICATION_PORT_BLOCK_ERROR, + properties: {}, + }) } + } else if ('refetchUsingHTTP' in data) { + setRefetchUsingHTTP('once') } } }