From 31ccfd7787171802d63c0bc030bb477261c13339 Mon Sep 17 00:00:00 2001 From: Seth Silesky Date: Mon, 26 Sep 2022 16:28:59 -0500 Subject: [PATCH] fix bug around dispatch --- .changeset/brown-islands-judge.md | 5 ++++ .../src/analytics/__tests__/dispatch.test.ts | 25 ------------------- packages/core/src/analytics/dispatch.ts | 3 +-- .../core/src/callback/__tests__/index.test.ts | 16 ++++++------ packages/core/src/callback/index.ts | 12 ++++----- 5 files changed, 19 insertions(+), 42 deletions(-) create mode 100644 .changeset/brown-islands-judge.md diff --git a/.changeset/brown-islands-judge.md b/.changeset/brown-islands-judge.md new file mode 100644 index 000000000..289c8df90 --- /dev/null +++ b/.changeset/brown-islands-judge.md @@ -0,0 +1,5 @@ +--- +'@segment/analytics-core': patch +--- + +Fix bug where delay and pTimeout are coupled diff --git a/packages/core/src/analytics/__tests__/dispatch.test.ts b/packages/core/src/analytics/__tests__/dispatch.test.ts index 9cf2ab61a..1111d0cf5 100644 --- a/packages/core/src/analytics/__tests__/dispatch.test.ts +++ b/packages/core/src/analytics/__tests__/dispatch.test.ts @@ -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, () => { diff --git a/packages/core/src/analytics/dispatch.ts b/packages/core/src/analytics/dispatch.ts index 0d90721f2..787b96511 100644 --- a/packages/core/src/analytics/dispatch.ts +++ b/packages/core/src/analytics/dispatch.ts @@ -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) { diff --git a/packages/core/src/callback/__tests__/index.test.ts b/packages/core/src/callback/__tests__/index.test.ts index dc6d562d8..8ceebf2d8 100644 --- a/packages/core/src/callback/__tests__/index.test.ts +++ b/packages/core/src/callback/__tests__/index.test.ts @@ -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 => { 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() diff --git a/packages/core/src/callback/index.ts b/packages/core/src/callback/index.ts index fb395502a..1fca70a94 100644 --- a/packages/core/src/callback/index.ts +++ b/packages/core/src/callback/index.ts @@ -22,16 +22,14 @@ function sleep(timeoutInMs: number): Promise { } /** - * @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 { const cb = () => { try { @@ -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')