Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add dispatch unit tests and minor refactoring #602

Merged
merged 6 commits into from
Sep 26, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
improve doc / descriptions
silesky committed Sep 25, 2022
commit e1da2c327f6e1a6b15468e57e32013be876d29aa
19 changes: 10 additions & 9 deletions packages/core/src/analytics/__tests__/dispatch.test.ts
Original file line number Diff line number Diff line change
@@ -16,7 +16,7 @@ jest.mock('../../callback', () => ({

import { EventQueue } from '../../queue/event-queue'
import { Emitter } from '../../emitter'
import { dispatch, getDelayTimeout } from '../dispatch'
import { dispatch, getDelay } from '../dispatch'
import { PriorityQueue } from '../../priority-queue'

let emitter!: Emitter
@@ -34,7 +34,7 @@ afterEach(() => {
})

describe('Dispatch', () => {
it('should return ctx if offline and retryQueue is false', async () => {
it('should not dispatch if client is currently offline and retries are *disabled* for the main event queue', async () => {
isOnline.mockReturnValue(false)
isOffline.mockReturnValue(true)

@@ -48,7 +48,7 @@ describe('Dispatch', () => {
expect(called).toBeFalsy()
})

it('should not dispatch if offline and retryQueue is true', async () => {
it('should be allowed to dispatch if client is currently offline and retries are *enabled* for the main event queue', async () => {
isOnline.mockReturnValue(false)
isOffline.mockReturnValue(true)

@@ -61,7 +61,7 @@ describe('Dispatch', () => {
expect(called).toBeTruthy()
})

it('should call dispatchSingle with ctx if queue is empty', async () => {
it('should call dispatchSingle correctly if queue is empty', async () => {
queue.isEmpty = jest.fn().mockReturnValue(true)
await dispatch({ type: 'screen' }, queue, emitter)
expect(dispatchSingleSpy).toBeCalledWith(
@@ -70,7 +70,7 @@ describe('Dispatch', () => {
expect(dispatchSpy).not.toBeCalled()
})

it('should call dispatch with ctx if queue is empty', async () => {
it('should call dispatch correctly if queue has items', async () => {
queue.isEmpty = jest.fn().mockReturnValue(false)
await dispatch({ type: 'screen' }, queue, emitter)
expect(dispatchSpy).toBeCalledWith(
@@ -90,14 +90,15 @@ describe('Dispatch', () => {
})
})

describe('getDelayTimeout', () => {
it('should work as expected', () => {
describe(getDelay, () => {
it('should calculate the amount of time to delay before invoking the callback', () => {
const aShortTimeAgo = Date.now() - 200
expect(Math.round(getDelayTimeout(aShortTimeAgo, 500))).toBe(300)
const timeout = 5000
expect(Math.round(getDelay(aShortTimeAgo, timeout))).toBe(4800)
})

it('should have a sensible default', () => {
const aShortTimeAgo = Date.now() - 200
expect(Math.round(getDelayTimeout(aShortTimeAgo))).toBe(100)
expect(Math.round(getDelay(aShortTimeAgo))).toBe(100)
})
})
8 changes: 3 additions & 5 deletions packages/core/src/analytics/dispatch.ts
Original file line number Diff line number Diff line change
@@ -13,11 +13,9 @@ type DispatchOptions = {
}

/* The amount of time in ms to wait before invoking the callback. */
export const getDelayTimeout = (
startTimeInEpochMS: number,
timeoutInMS?: number
) => {
export const getDelay = (startTimeInEpochMS: number, timeoutInMS?: number) => {
const elapsedTime = Date.now() - startTimeInEpochMS
// increasing the timeout increases the delay by almost the same amount -- this is weird legacy behavior.
return Math.max((timeoutInMS ?? 300) - elapsedTime, 0)
}
/**
@@ -53,7 +51,7 @@ export async function dispatch(
dispatched = await invokeCallback(
dispatched,
options.callback,
getDelayTimeout(startTime, options.timeout),
getDelay(startTime, options.timeout), // TODO: I have no idea why this delay is based on the timeout -- so if you set a timeout of 5seconds, it will (in addition) actually delay ~5seconds before invoking.
options.timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense. So normally 300ms timeout, but let user configure the value to whatever they want?

)
}
8 changes: 5 additions & 3 deletions packages/core/src/callback/index.ts
Original file line number Diff line number Diff line change
@@ -22,13 +22,15 @@ function sleep(timeoutInMs: number): Promise<void> {
}

/**
* @param delayTimeout - The amount of time in ms to wait before invoking the callback.
* @param delay - The amount of time in ms to wait before invoking the callback.
* Ajs 1.0 worked differently so the delay was meant to give ajs a chance to send the event before invoking the callback.
* Customers used that behavior to delay navigating to a new page. However, AJS 2.0 resolves a promise once the event has already been sent... so this is likely used to give third party destinations time to flush
* @param timeout - The maximum amount of time in ms to allow the callback to run for.
*/
export function invokeCallback(
ctx: CoreContext,
callback: Callback,
delayTimeout: number,
delay: number,
timeout?: number
): Promise<CoreContext> {
const cb = () => {
@@ -40,7 +42,7 @@ export function invokeCallback(
}

return (
sleep(delayTimeout)
sleep(delay)
// pTimeout ensures that the callback can't cause the context to hang
.then(() => pTimeout(cb(), timeout ?? 1000))
.catch((err) => {