Skip to content

Commit

Permalink
fix bug around dispatch
Browse files Browse the repository at this point in the history
  • Loading branch information
silesky committed Sep 26, 2022
1 parent 8ddc550 commit 31ccfd7
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 42 deletions.
5 changes: 5 additions & 0 deletions .changeset/brown-islands-judge.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@segment/analytics-core': patch
---

Fix bug where delay and pTimeout are coupled
25 changes: 0 additions & 25 deletions packages/core/src/analytics/__tests__/dispatch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,31 +109,6 @@ describe('Dispatch', () => {
expect(ctx).toEqual(screenCtxMatcher)
expect(_cb).toBe(cb)
})

// TODO: Inconsistent behavior? This seems like a bug.
it('should have inconsistent timeout behavior where the delay is different based on whether timeout is explicitly set to 1000 or not', async () => {
{
const TIMEOUT = 1000
await dispatch({ type: 'screen' }, queue, emitter, {
callback: jest.fn(),
timeout: TIMEOUT,
})
const [, , delay] = invokeCallback.mock.calls[0]
expect(delay).toBeGreaterThan(990) // ???
expect(delay).toBeLessThanOrEqual(1000) // ???
}
{
invokeCallback.mockReset()
const TIMEOUT = undefined // this defaults to 1000 in the invokeCallback function
await dispatch({ type: 'screen' }, queue, emitter, {
callback: jest.fn(),
timeout: TIMEOUT,
})
const [, , delay] = invokeCallback.mock.calls[0]
expect(delay).toBeGreaterThan(290) // ???
expect(delay).toBeLessThanOrEqual(300) // ???
}
})
})

describe(getDelay, () => {
Expand Down
3 changes: 1 addition & 2 deletions packages/core/src/analytics/dispatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ export async function dispatch(
dispatched = await invokeCallback(
dispatched,
options.callback,
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
getDelay(startTime, options.timeout)
)
}
if (options?.debug) {
Expand Down
16 changes: 8 additions & 8 deletions packages/core/src/callback/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,37 @@ describe(invokeCallback, () => {
})

// Fixes GitHub issue: https://github.com/segmentio/analytics-next/issues/409
// A.JS classic waited for the timeout before invoking callback,
// A.JS classic waited for the timeout/delay before invoking callback,
// so keep same behavior in A.JS next.
it('calls the callback after a timeout', async () => {
it('calls the callback after a delay', async () => {
const ctx = new CoreContext({
type: 'track',
})

const fn = jest.fn()
const timeout = 100
const delay = 100

const startTime = Date.now()
const returned = await invokeCallback(ctx, fn, timeout)
const returned = await invokeCallback(ctx, fn, delay)
const endTime = Date.now()

expect(fn).toHaveBeenCalled()
expect(endTime - startTime).toBeGreaterThanOrEqual(timeout - 1)
expect(endTime - startTime).toBeGreaterThanOrEqual(delay - 1)
expect(returned).toBe(ctx)
})

it('ignores the callback after a timeout', async () => {
it('ignores the callback if it takes too long to resolve', async () => {
const ctx = new CoreContext({
type: 'track',
})

const slow = (_ctx: CoreContext): Promise<void> => {
return new Promise((resolve) => {
setTimeout(resolve, 200)
setTimeout(resolve, 1100)
})
}

const returned = await invokeCallback(ctx, slow, 0, 50)
const returned = await invokeCallback(ctx, slow, 0)
expect(returned).toBe(ctx)

const logs = returned.logs()
Expand Down
12 changes: 5 additions & 7 deletions packages/core/src/callback/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,14 @@ function sleep(timeoutInMs: number): Promise<void> {
}

/**
* @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.
* @param ctx
* @param callback - the function to invoke
* @param delay - aka "timeout". The amount of time in ms to wait before invoking the callback.
*/
export function invokeCallback(
ctx: CoreContext,
callback: Callback,
delay: number,
timeout?: number
delay: number
): Promise<CoreContext> {
const cb = () => {
try {
Expand All @@ -44,7 +42,7 @@ export function invokeCallback(
return (
sleep(delay)
// pTimeout ensures that the callback can't cause the context to hang
.then(() => pTimeout(cb(), timeout ?? 1000))
.then(() => pTimeout(cb(), 1000))
.catch((err) => {
ctx?.log('warn', 'Callback Error', { error: err })
ctx?.stats?.increment('callback_error')
Expand Down

0 comments on commit 31ccfd7

Please sign in to comment.