Skip to content

Commit

Permalink
refactor(app): refine useNotifyService
Browse files Browse the repository at this point in the history
useNotifyService leaks refetch frequencies unnecessarily. We can refactor this logic to
useNotifyService.
  • Loading branch information
mjhuff committed May 6, 2024
1 parent 3b2e900 commit 16a871f
Show file tree
Hide file tree
Showing 10 changed files with 73 additions and 111 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ import { handleTipsAttachedModal } from '../TipsAttachedModal'
import { LEFT } from '@opentrons/shared-data'
import { mockPipetteInfo } from '../../../redux/pipettes/__fixtures__'
import { ROBOT_MODEL_OT3 } from '../../../redux/discovery'
import { useNotifyCurrentMaintenanceRun } from '../../../resources/maintenance_runs'
import { useCloseCurrentRun } from '../../ProtocolUpload/hooks'

import type { PipetteModelSpecs } from '@opentrons/shared-data'
import type { HostConfig } from '@opentrons/api-client'

vi.mock('../../../resources/maintenance_runs')
vi.mock('../../../resources/useNotifyService')
vi.mock('../../ProtocolUpload/hooks')

const MOCK_ACTUAL_PIPETTE = {
...mockPipetteInfo.pipetteSpecs,
Expand Down Expand Up @@ -53,12 +52,8 @@ const render = (pipetteSpecs: PipetteModelSpecs) => {

describe('TipsAttachedModal', () => {
beforeEach(() => {
vi.mocked(useNotifyCurrentMaintenanceRun).mockReturnValue({
data: {
data: {
id: 'test',
},
},
vi.mocked(useCloseCurrentRun).mockReturnValue({
closeCurrentRun: vi.fn(),
} as any)
})

Expand Down
41 changes: 17 additions & 24 deletions app/src/resources/__tests__/useNotifyService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ describe('useNotifyService', () => {

beforeEach(() => {
mockDispatch = vi.fn()
mockHTTPRefetch = vi.fn()
mockTrackEvent = vi.fn()
vi.mocked(useTrackEvent).mockReturnValue(mockTrackEvent)
vi.mocked(useDispatch).mockReturnValue(mockDispatch)
Expand All @@ -48,55 +47,51 @@ describe('useNotifyService', () => {
})

it('should trigger an HTTP refetch and subscribe action on a successful initial mount', () => {
renderHook(() =>
const { result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: MOCK_OPTIONS,
} as any)
)
expect(mockHTTPRefetch).toHaveBeenCalledWith('once')
expect(result.current.isNotifyEnabled).toEqual(true)
expect(mockDispatch).toHaveBeenCalledWith(
notifySubscribeAction(MOCK_HOST_CONFIG.hostname, MOCK_TOPIC)
)
expect(appShellListener).toHaveBeenCalled()
})

it('should not subscribe to notifications if forceHttpPolling is true', () => {
renderHook(() =>
const { result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: { ...MOCK_OPTIONS, forceHttpPolling: true },
} as any)
)
expect(mockHTTPRefetch).toHaveBeenCalled()
expect(result.current.isNotifyEnabled).toEqual(true)
expect(appShellListener).not.toHaveBeenCalled()
expect(mockDispatch).not.toHaveBeenCalled()
})

it('should not subscribe to notifications if enabled is false', () => {
renderHook(() =>
const { result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: { ...MOCK_OPTIONS, enabled: false },
} as any)
)
expect(mockHTTPRefetch).toHaveBeenCalled()
expect(result.current.isNotifyEnabled).toEqual(true)
expect(appShellListener).not.toHaveBeenCalled()
expect(mockDispatch).not.toHaveBeenCalled()
})

it('should not subscribe to notifications if staleTime is Infinity', () => {
renderHook(() =>
const { result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: { ...MOCK_OPTIONS, staleTime: Infinity },
} as any)
)
expect(mockHTTPRefetch).toHaveBeenCalled()
expect(result.current.isNotifyEnabled).toEqual(true)
expect(appShellListener).not.toHaveBeenCalled()
expect(mockDispatch).not.toHaveBeenCalled()
})
Expand All @@ -106,14 +101,15 @@ describe('useNotifyService', () => {
const errorSpy = vi.spyOn(console, 'error')
errorSpy.mockImplementation(() => {})

renderHook(() =>
const { result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: MOCK_OPTIONS,
} as any)
)
expect(mockHTTPRefetch).toHaveBeenCalledWith('always')

expect(result.current.isNotifyEnabled).toEqual(true)
})

it('should return set HTTP refetch to always and fire an analytics reporting event if the connection was refused', () => {
Expand All @@ -123,7 +119,7 @@ describe('useNotifyService', () => {
// eslint-disable-next-line n/no-callback-literal
callback('ECONNREFUSED')
})
const { rerender } = renderHook(() =>
const { rerender, result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
Expand All @@ -132,7 +128,7 @@ describe('useNotifyService', () => {
)
expect(mockTrackEvent).toHaveBeenCalled()
rerender()
expect(mockHTTPRefetch).toHaveBeenCalledWith('always')
expect(result.current.isNotifyEnabled).toEqual(true)
})

it('should trigger a single HTTP refetch if the refetch flag was returned', () => {
Expand All @@ -142,15 +138,15 @@ describe('useNotifyService', () => {
// eslint-disable-next-line n/no-callback-literal
callback({ refetch: true })
})
const { rerender } = renderHook(() =>
const { rerender, result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: MOCK_OPTIONS,
} as any)
)
rerender()
expect(mockHTTPRefetch).toHaveBeenCalledWith('once')
expect(result.current.isNotifyEnabled).toEqual(true)
})

it('should trigger a single HTTP refetch if the unsubscribe flag was returned', () => {
Expand All @@ -160,22 +156,20 @@ describe('useNotifyService', () => {
// eslint-disable-next-line n/no-callback-literal
callback({ unsubscribe: true })
})
const { rerender } = renderHook(() =>
const { rerender, result } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: MOCK_OPTIONS,
} as any)
)
rerender()
expect(mockHTTPRefetch).toHaveBeenCalledWith('once')
expect(result.current.isNotifyEnabled).toEqual(true)
})

it('should clean up the listener on dismount', () => {
const { unmount } = renderHook(() =>
useNotifyService({
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: MOCK_OPTIONS,
})
)
Expand All @@ -188,7 +182,6 @@ describe('useNotifyService', () => {
useNotifyService({
hostOverride: MOCK_HOST_CONFIG,
topic: MOCK_TOPIC,
setRefetch: mockHTTPRefetch,
options: MOCK_OPTIONS,
})
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,23 @@
import * as React from 'react'

import { useDeckConfigurationQuery } from '@opentrons/react-api-client'

import { useNotifyService } from '../useNotifyService'

import type { UseQueryResult } from 'react-query'
import type { DeckConfiguration } from '@opentrons/shared-data'
import type {
QueryOptionsWithPolling,
HTTPRefetchFrequency,
} from '../useNotifyService'
import type { QueryOptionsWithPolling } from '../useNotifyService'

export function useNotifyDeckConfigurationQuery(
options: QueryOptionsWithPolling<DeckConfiguration, unknown> = {}
): UseQueryResult<DeckConfiguration> {
const [refetch, setRefetch] = React.useState<HTTPRefetchFrequency>(null)

useNotifyService<DeckConfiguration, unknown>({
const { notifyOnSettled, isNotifyEnabled } = useNotifyService({
topic: 'robot-server/deck_configuration',
setRefetch,
options,
})

const httpQueryResult = useDeckConfigurationQuery({
...options,
enabled: options?.enabled !== false && refetch != null,
onSettled: refetch === 'once' ? () => setRefetch(null) : () => null,
enabled: options?.enabled !== false && isNotifyEnabled,
onSettled: notifyOnSettled,
})

return httpQueryResult
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,23 @@
import * as React from 'react'

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,
HTTPRefetchFrequency,
} from '../useNotifyService'
import type { QueryOptionsWithPolling } from '../useNotifyService'

export function useNotifyCurrentMaintenanceRun(
options: QueryOptionsWithPolling<MaintenanceRun, Error> = {}
): UseQueryResult<MaintenanceRun> | UseQueryResult<MaintenanceRun, Error> {
const [refetch, setRefetch] = React.useState<HTTPRefetchFrequency>(null)

useNotifyService<MaintenanceRun, Error>({
const { notifyOnSettled, isNotifyEnabled } = useNotifyService({
topic: 'robot-server/maintenance_runs/current_run',
setRefetch,
options,
})

const httpQueryResult = useCurrentMaintenanceRun({
...options,
enabled: options?.enabled !== false && refetch != null,
onSettled: refetch === 'once' ? () => setRefetch(null) : () => null,
enabled: options?.enabled !== false && isNotifyEnabled,
onSettled: notifyOnSettled,
})

return httpQueryResult
Expand Down
16 changes: 4 additions & 12 deletions app/src/resources/runs/useNotifyAllCommandsAsPreSerializedList.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,26 @@
import * as React from 'react'

import { useAllCommandsAsPreSerializedList } from '@opentrons/react-api-client'

import { useNotifyService } from '../useNotifyService'

import type { UseQueryResult } from 'react-query'
import type { AxiosError } from 'axios'
import type { CommandsData, GetCommandsParams } from '@opentrons/api-client'
import type {
QueryOptionsWithPolling,
HTTPRefetchFrequency,
} from '../useNotifyService'
import type { QueryOptionsWithPolling } from '../useNotifyService'

export function useNotifyAllCommandsAsPreSerializedList(
runId: string | null,
params?: GetCommandsParams | null,
options: QueryOptionsWithPolling<CommandsData, AxiosError> = {}
): UseQueryResult<CommandsData, AxiosError> {
const [refetch, setRefetch] = React.useState<HTTPRefetchFrequency>(null)

useNotifyService<CommandsData, AxiosError>({
const { notifyOnSettled, isNotifyEnabled } = useNotifyService({
topic: `robot-server/runs/pre_serialized_commands/${runId}`,
setRefetch,
options,
})

const httpResponse = useAllCommandsAsPreSerializedList(runId, params, {
...options,
enabled: options?.enabled !== false && refetch != null,
onSettled: refetch === 'once' ? () => setRefetch(null) : () => null,
enabled: options?.enabled !== false && isNotifyEnabled,
onSettled: notifyOnSettled,
})

return httpResponse
Expand Down
16 changes: 4 additions & 12 deletions app/src/resources/runs/useNotifyAllRunsQuery.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import * as React from 'react'

import { useAllRunsQuery } from '@opentrons/react-api-client'

import { useNotifyService } from '../useNotifyService'
Expand All @@ -8,21 +6,15 @@ 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,
HTTPRefetchFrequency,
} from '../useNotifyService'
import type { QueryOptionsWithPolling } from '../useNotifyService'

export function useNotifyAllRunsQuery(
params: GetRunsParams = {},
options: QueryOptionsWithPolling<UseAllRunsQueryOptions, AxiosError> = {},
hostOverride?: HostConfig | null
): UseQueryResult<Runs, AxiosError> {
const [refetch, setRefetch] = React.useState<HTTPRefetchFrequency>(null)

useNotifyService<UseAllRunsQueryOptions, AxiosError>({
const { notifyOnSettled, isNotifyEnabled } = useNotifyService({
topic: 'robot-server/runs',
setRefetch,
options,
hostOverride,
})
Expand All @@ -31,8 +23,8 @@ export function useNotifyAllRunsQuery(
params,
{
...(options as UseAllRunsQueryOptions),
enabled: options?.enabled !== false && refetch != null,
onSettled: refetch === 'once' ? () => setRefetch(null) : () => null,
enabled: options?.enabled !== false && isNotifyEnabled,
onSettled: notifyOnSettled,
},
hostOverride
)
Expand Down
16 changes: 4 additions & 12 deletions app/src/resources/runs/useNotifyLastRunCommand.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,22 @@
import * as React from 'react'

import { useNotifyService } from '../useNotifyService'
import { useLastRunCommand } from '../../organisms/Devices/hooks/useLastRunCommand'

import type { CommandsData, RunCommandSummary } from '@opentrons/api-client'
import type {
QueryOptionsWithPolling,
HTTPRefetchFrequency,
} from '../useNotifyService'
import type { QueryOptionsWithPolling } from '../useNotifyService'

export function useNotifyLastRunCommand(
runId: string,
options: QueryOptionsWithPolling<CommandsData, Error> = {}
): RunCommandSummary | null {
const [refetch, setRefetch] = React.useState<HTTPRefetchFrequency>(null)

useNotifyService({
const { notifyOnSettled, isNotifyEnabled } = useNotifyService({
topic: 'robot-server/runs/current_command',
setRefetch,
options,
})

const httpResponse = useLastRunCommand(runId, {
...options,
enabled: options?.enabled !== false && refetch != null,
onSettled: refetch === 'once' ? () => setRefetch(null) : () => null,
enabled: options?.enabled !== false && isNotifyEnabled,
onSettled: notifyOnSettled,
})

return httpResponse
Expand Down
Loading

0 comments on commit 16a871f

Please sign in to comment.