From 70c124ad895dd17aaf2c0aa01fb2ed95f8905a8f Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Jul 2023 15:17:40 -0500 Subject: [PATCH 01/24] Seth - example refactor wip wip wip wip wip wip wip wip wip wip wip --- .changeset/nervous-chefs-talk.md | 5 ++ packages/node/src/__tests__/callback.test.ts | 20 ++--- .../src/__tests__/disable.integration.test.ts | 21 +++-- .../src/__tests__/emitter.integration.test.ts | 21 +++-- .../graceful-shutdown-integration.test.ts | 20 +++-- .../src/__tests__/http-integration.test.ts | 13 +-- .../node/src/__tests__/integration.test.ts | 30 ++++--- packages/node/src/__tests__/plugins.test.ts | 8 -- .../test-helpers/create-test-analytics.ts | 41 ++++++++- packages/node/src/__tests__/typedef-tests.ts | 22 ++++- packages/node/src/app/analytics-node.ts | 2 + packages/node/src/app/settings.ts | 5 ++ packages/node/src/index.ts | 7 ++ packages/node/src/lib/__tests__/abort.test.ts | 1 - packages/node/src/lib/fetch.ts | 10 ++- packages/node/src/lib/http-client.ts | 87 +++++++++++++++++++ .../segmentio/__tests__/methods.test.ts | 68 ++++++--------- .../segmentio/__tests__/publisher.test.ts | 57 ++++-------- .../node/src/plugins/segmentio/publisher.ts | 47 ++++------ 19 files changed, 305 insertions(+), 180 deletions(-) create mode 100644 .changeset/nervous-chefs-talk.md create mode 100644 packages/node/src/lib/http-client.ts diff --git a/.changeset/nervous-chefs-talk.md b/.changeset/nervous-chefs-talk.md new file mode 100644 index 000000000..0a410b893 --- /dev/null +++ b/.changeset/nervous-chefs-talk.md @@ -0,0 +1,5 @@ +--- +'@segment/analytics-node': minor +--- + +Add `httpClient` setting. This allow users to override default HTTP client with a custom one. diff --git a/packages/node/src/__tests__/callback.test.ts b/packages/node/src/__tests__/callback.test.ts index 426171ec8..c9aa6d80f 100644 --- a/packages/node/src/__tests__/callback.test.ts +++ b/packages/node/src/__tests__/callback.test.ts @@ -1,17 +1,11 @@ -const fetcher = jest.fn() -jest.mock('../lib/fetch', () => ({ fetch: fetcher })) - -import { createError, createSuccess } from './test-helpers/factories' import { createTestAnalytics } from './test-helpers/create-test-analytics' import { Context } from '../app/context' describe('Callback behavior', () => { - beforeEach(() => { - fetcher.mockReturnValue(createSuccess()) - }) - it('should handle success', async () => { - const ajs = createTestAnalytics({ maxEventsInBatch: 1 }) + const ajs = createTestAnalytics({ + maxEventsInBatch: 1, + }) const ctx = await new Promise((resolve, reject) => ajs.track( { @@ -29,8 +23,12 @@ describe('Callback behavior', () => { }) it('should handle errors', async () => { - fetcher.mockReturnValue(createError()) - const ajs = createTestAnalytics({ maxEventsInBatch: 1 }) + const ajs = createTestAnalytics( + { + maxEventsInBatch: 1, + }, + { withError: true } + ) const [err, ctx] = await new Promise<[any, Context]>((resolve) => ajs.track( { diff --git a/packages/node/src/__tests__/disable.integration.test.ts b/packages/node/src/__tests__/disable.integration.test.ts index 730170069..3a8b09c8b 100644 --- a/packages/node/src/__tests__/disable.integration.test.ts +++ b/packages/node/src/__tests__/disable.integration.test.ts @@ -1,9 +1,12 @@ -const fetcher = jest.fn() -jest.mock('../lib/fetch', () => ({ fetch: fetcher })) - -import { createTestAnalytics } from './test-helpers/create-test-analytics' +import { + createTestAnalytics, + TestFetchClient, +} from './test-helpers/create-test-analytics' describe('disable', () => { + const httpClient = new TestFetchClient() + const mockSend = jest.spyOn(httpClient, 'send') + it('should dispatch callbacks and emit an http request, even if disabled', async () => { const analytics = createTestAnalytics({ disable: true, @@ -16,22 +19,24 @@ describe('disable', () => { expect(emitterCb).toBeCalledTimes(1) }) - it('should call fetch if disabled is false', async () => { + it('should call .send if disabled is false', async () => { const analytics = createTestAnalytics({ disable: false, + httpClient: httpClient, }) await new Promise((resolve) => analytics.track({ anonymousId: 'foo', event: 'bar' }, resolve) ) - expect(fetcher).toBeCalled() + expect(mockSend).toBeCalledTimes(1) }) - it('should not call fetch if disabled is true', async () => { + it('should not call .send if disabled is true', async () => { const analytics = createTestAnalytics({ disable: true, + httpClient: httpClient, }) await new Promise((resolve) => analytics.track({ anonymousId: 'foo', event: 'bar' }, resolve) ) - expect(fetcher).not.toBeCalled() + expect(mockSend).not.toBeCalled() }) }) diff --git a/packages/node/src/__tests__/emitter.integration.test.ts b/packages/node/src/__tests__/emitter.integration.test.ts index 0282e29d8..3f86e59ac 100644 --- a/packages/node/src/__tests__/emitter.integration.test.ts +++ b/packages/node/src/__tests__/emitter.integration.test.ts @@ -1,13 +1,8 @@ -const fetcher = jest.fn() -jest.mock('../lib/fetch', () => ({ fetch: fetcher })) - -import { createError, createSuccess } from './test-helpers/factories' import { createTestAnalytics } from './test-helpers/create-test-analytics' import { assertHttpRequestEmittedEvent } from './test-helpers/assert-shape' describe('http_request', () => { it('emits an http_request event if success', async () => { - fetcher.mockReturnValue(createSuccess()) const analytics = createTestAnalytics() const fn = jest.fn() analytics.on('http_request', fn) @@ -19,8 +14,12 @@ describe('http_request', () => { }) it('emits an http_request event if error', async () => { - fetcher.mockReturnValue(createError()) - const analytics = createTestAnalytics({ maxRetries: 0 }) + const analytics = createTestAnalytics( + { + maxRetries: 0, + }, + { withError: true } + ) const fn = jest.fn() analytics.on('http_request', fn) await new Promise((resolve) => @@ -30,8 +29,12 @@ describe('http_request', () => { }) it('if error, emits an http_request event on every retry', async () => { - fetcher.mockReturnValue(createError()) - const analytics = createTestAnalytics({ maxRetries: 2 }) + const analytics = createTestAnalytics( + { + maxRetries: 2, + }, + { withError: true } + ) const fn = jest.fn() analytics.on('http_request', fn) await new Promise((resolve) => diff --git a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts index bd942ab3a..86913d384 100644 --- a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts +++ b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts @@ -1,9 +1,5 @@ -import { createSuccess } from './test-helpers/factories' +import { TestFetchClient } from './test-helpers/create-test-analytics' import { performance as perf } from 'perf_hooks' - -const fetcher = jest.fn() -jest.mock('../lib/fetch', () => ({ fetch: fetcher })) - import { Analytics } from '../app/analytics-node' import { sleep } from './test-helpers/sleep' import { Plugin, SegmentEvent } from '../app/types' @@ -17,22 +13,25 @@ const testPlugin: Plugin = { isLoaded: () => true, } +const testClient = new TestFetchClient() +const sendSpy = jest.spyOn(testClient, 'send') + describe('Ability for users to exit without losing events', () => { let ajs!: Analytics beforeEach(async () => { - fetcher.mockReturnValue(createSuccess()) ajs = new Analytics({ writeKey: 'abc123', maxEventsInBatch: 1, + httpClient: testClient, }) }) const _helpers = { - getFetchCalls: (mockedFetchFn = fetcher) => - mockedFetchFn.mock.calls.map(([url, request]) => ({ + getFetchCalls: () => + sendSpy.mock.calls.map(([url, request]) => ({ url, method: request.method, headers: request.headers, - body: JSON.parse(request.body), + body: JSON.parse(request.body!), })), makeTrackCall: (analytics = ajs, cb?: (...args: any[]) => void) => { analytics.track({ userId: 'foo', event: 'Thing Updated' }, cb) @@ -89,6 +88,7 @@ describe('Ability for users to exit without losing events', () => { ajs = new Analytics({ writeKey: 'abc123', flushInterval, + httpClient: testClient, }) const closeAndFlushTimeout = ajs['_closeAndFlushDefaultTimeout'] expect(closeAndFlushTimeout).toBe(flushInterval * 1.25) @@ -190,6 +190,7 @@ describe('Ability for users to exit without losing events', () => { writeKey: 'foo', flushInterval: 10000, maxEventsInBatch: 15, + httpClient: testClient, }) _helpers.makeTrackCall(analytics) _helpers.makeTrackCall(analytics) @@ -220,6 +221,7 @@ describe('Ability for users to exit without losing events', () => { writeKey: 'foo', flushInterval: 10000, maxEventsInBatch: 15, + httpClient: testClient, }) await analytics.register(_testPlugin) _helpers.makeTrackCall(analytics) diff --git a/packages/node/src/__tests__/http-integration.test.ts b/packages/node/src/__tests__/http-integration.test.ts index 1599ba705..aba7d35be 100644 --- a/packages/node/src/__tests__/http-integration.test.ts +++ b/packages/node/src/__tests__/http-integration.test.ts @@ -33,7 +33,7 @@ describe('Method Smoke Tests', () => { let scope: nock.Scope let ajs: Analytics beforeEach(async () => { - ajs = createTestAnalytics() + ajs = createTestAnalytics({}, { useRealHTTPClient: true }) }) describe('Metadata', () => { @@ -333,10 +333,13 @@ describe('Client: requestTimeout', () => { }) it('should timeout immediately if request timeout is set to 0', async () => { jest.useRealTimers() - const ajs = createTestAnalytics({ - maxEventsInBatch: 1, - httpRequestTimeout: 0, - }) + const ajs = createTestAnalytics( + { + maxEventsInBatch: 1, + httpRequestTimeout: 0, + }, + { useRealHTTPClient: true } + ) ajs.track({ event: 'foo', userId: 'foo', properties: { hello: 'world' } }) try { await resolveCtx(ajs, 'track') diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index 0bf909bad..578fc7bae 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -1,19 +1,18 @@ -const fetcher = jest.fn() -jest.mock('../lib/fetch', () => ({ fetch: fetcher })) - import { Plugin } from '../app/types' import { resolveCtx } from './test-helpers/resolve-ctx' import { testPlugin } from './test-helpers/test-plugin' -import { createSuccess, createError } from './test-helpers/factories' -import { createTestAnalytics } from './test-helpers/create-test-analytics' +import { createError } from './test-helpers/factories' +import { + createTestAnalytics, + TestFetchClient, +} from './test-helpers/create-test-analytics' const writeKey = 'foo' jest.setTimeout(10000) const timestamp = new Date() -beforeEach(() => { - fetcher.mockReturnValue(createSuccess()) -}) +const testClient = new TestFetchClient() +const sendSpy = jest.spyOn(testClient, 'send') describe('Settings / Configuration Init', () => { it('throws if no writeKey', () => { @@ -28,11 +27,12 @@ describe('Settings / Configuration Init', () => { const analytics = createTestAnalytics({ host: 'http://foo.com', path: '/bar', + httpClient: testClient, }) const track = resolveCtx(analytics, 'track') analytics.track({ event: 'foo', userId: 'sup' }) await track - expect(fetcher.mock.calls[0][0]).toBe('http://foo.com/bar') + expect(sendSpy.mock.calls[0][0]).toBe('http://foo.com/bar') }) it('throws if host / path is bad', async () => { @@ -53,10 +53,14 @@ describe('Error handling', () => { }) it('should emit on an error', async () => { - const analytics = createTestAnalytics({ maxRetries: 0 }) - fetcher.mockReturnValue( - createError({ statusText: 'Service Unavailable', status: 503 }) - ) + const err = createError({ + statusText: 'Service Unavailable', + status: 503, + }) + const analytics = createTestAnalytics({ + maxRetries: 0, + httpClient: new TestFetchClient({ response: err }), + }) try { const promise = resolveCtx(analytics, 'track') analytics.track({ event: 'foo', userId: 'sup' }) diff --git a/packages/node/src/__tests__/plugins.test.ts b/packages/node/src/__tests__/plugins.test.ts index 4efa6b95e..18a3ef8c9 100644 --- a/packages/node/src/__tests__/plugins.test.ts +++ b/packages/node/src/__tests__/plugins.test.ts @@ -1,14 +1,6 @@ -const fetcher = jest.fn() -jest.mock('../lib/fetch', () => ({ fetch: fetcher })) - -import { createSuccess } from './test-helpers/factories' import { createTestAnalytics } from './test-helpers/create-test-analytics' describe('Plugins', () => { - beforeEach(() => { - fetcher.mockReturnValue(createSuccess()) - }) - describe('Initialize', () => { it('loads analytics-node-next plugin', async () => { const analytics = createTestAnalytics() diff --git a/packages/node/src/__tests__/test-helpers/create-test-analytics.ts b/packages/node/src/__tests__/test-helpers/create-test-analytics.ts index e97e35f5a..d3c7203d9 100644 --- a/packages/node/src/__tests__/test-helpers/create-test-analytics.ts +++ b/packages/node/src/__tests__/test-helpers/create-test-analytics.ts @@ -1,8 +1,45 @@ import { Analytics } from '../../app/analytics-node' import { AnalyticsSettings } from '../../app/settings' +import { AnalyticsHTTPClientDELETE } from '../../lib/http-client' +import { createError, createSuccess } from './factories' export const createTestAnalytics = ( - settings: Partial = {} + settings: Partial = {}, + { + withError, + useRealHTTPClient, + }: TestFetchClientOptions & { useRealHTTPClient?: boolean } = {} ) => { - return new Analytics({ writeKey: 'foo', flushInterval: 100, ...settings }) + return new Analytics({ + writeKey: 'foo', + flushInterval: 100, + ...(useRealHTTPClient + ? {} + : { httpClient: new TestFetchClient({ withError }) }), + ...settings, + }) +} + +export type TestFetchClientOptions = { + withError?: boolean + /** override response (if needed) */ + response?: Response | Promise +} + +/** + * Test http client. By default, it will return a successful response. + */ +export class TestFetchClient implements AnalyticsHTTPClientDELETE { + private withError?: TestFetchClientOptions['withError'] + private response?: TestFetchClientOptions['response'] + constructor({ withError, response }: TestFetchClientOptions = {}) { + this.withError = withError + this.response = response + } + send(..._args: Parameters) { + if (this.response) { + return Promise.resolve(this.response) + } + return Promise.resolve(this.withError ? createError() : createSuccess()) + } } diff --git a/packages/node/src/__tests__/typedef-tests.ts b/packages/node/src/__tests__/typedef-tests.ts index 01baa9564..c7d414b9c 100644 --- a/packages/node/src/__tests__/typedef-tests.ts +++ b/packages/node/src/__tests__/typedef-tests.ts @@ -1,4 +1,11 @@ -import { Analytics, Context, Plugin, UserTraits, GroupTraits } from '../' +import { + Analytics, + Context, + Plugin, + UserTraits, + GroupTraits, + AnalyticsHTTPClientDELETE, +} from '../' /** * These are general typescript definition tests; @@ -14,6 +21,19 @@ export default { analytics.VERSION = 'foo' }, + 'httpClient setting should be compatible with standard fetch and node-fetch interface': + () => { + const nodeFetchClient: AnalyticsHTTPClientDELETE = { + send: require('node-fetch'), + } + new Analytics({ writeKey: 'foo', httpClient: nodeFetchClient }) + + const standardFetchClient: AnalyticsHTTPClientDELETE = { + send: fetch, + } + new Analytics({ writeKey: 'foo', httpClient: standardFetchClient }) + }, + 'track/id/pg/screen/grp calls should require either userId or anonymousId': () => { const analytics = new Analytics({ writeKey: 'abc' }) diff --git a/packages/node/src/app/analytics-node.ts b/packages/node/src/app/analytics-node.ts index 25f7f4d42..a3219886c 100644 --- a/packages/node/src/app/analytics-node.ts +++ b/packages/node/src/app/analytics-node.ts @@ -16,6 +16,7 @@ import { } from './types' import { Context } from './context' import { NodeEventQueue } from './event-queue' +import { fetch } from '../lib/fetch' export class Analytics extends NodeEmitter implements CoreAnalytics { private readonly _eventFactory: NodeEventFactory @@ -51,6 +52,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { httpRequestTimeout: settings.httpRequestTimeout, disable: settings.disable, flushInterval, + httpFetchFn: settings.httpClient ?? fetch, }, this as NodeEmitter ) diff --git a/packages/node/src/app/settings.ts b/packages/node/src/app/settings.ts index 8088bc56e..7b87f4484 100644 --- a/packages/node/src/app/settings.ts +++ b/packages/node/src/app/settings.ts @@ -1,4 +1,5 @@ import { ValidationError } from '@segment/analytics-core' +import { HTTPFetchFn } from '../lib/http-client' export interface AnalyticsSettings { /** @@ -34,6 +35,10 @@ export interface AnalyticsSettings { * Disable the analytics library. All calls will be a noop. Default: false. */ disable?: boolean + /** + * Supply a default http client implementation (such as one supporting proxy). Default: The value of globalThis.fetch, with node-fetch as a fallback. + */ + httpClient?: HTTPFetchFn } export const validateSettings = (settings: AnalyticsSettings) => { diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index d6294449a..9701b6365 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -1,5 +1,12 @@ export { Analytics } from './app/analytics-node' export { Context } from './app/context' +export type { + FetchHTTPClient, + AnalyticsHTTPClientDELETE, + HTTPClientOptions, + HTTPFetchFn, + HTTPFetchClientResponse, +} from './lib/http-client' export type { Plugin, GroupTraits, diff --git a/packages/node/src/lib/__tests__/abort.test.ts b/packages/node/src/lib/__tests__/abort.test.ts index ecc4af764..82a4336f2 100644 --- a/packages/node/src/lib/__tests__/abort.test.ts +++ b/packages/node/src/lib/__tests__/abort.test.ts @@ -1,6 +1,5 @@ import { abortSignalAfterTimeout } from '../abort' import nock from 'nock' -import { fetch } from '../fetch' import { sleep } from '@segment/analytics-core' describe(abortSignalAfterTimeout, () => { diff --git a/packages/node/src/lib/fetch.ts b/packages/node/src/lib/fetch.ts index 35e65d69f..46b764cb1 100644 --- a/packages/node/src/lib/fetch.ts +++ b/packages/node/src/lib/fetch.ts @@ -1,11 +1,13 @@ -export const fetch: typeof globalThis.fetch = async (...args) => { +import type { HTTPFetchFn } from './http-client' + +export const fetch: HTTPFetchFn = async (...args) => { if (globalThis.fetch) { return globalThis.fetch(...args) - } // @ts-ignore + } // This guard causes is important, as it causes dead-code elimination to be enabled inside this block. + // @ts-ignore else if (typeof EdgeRuntime !== 'string') { - // @ts-ignore - return (await import('node-fetch')).default(...args) as Response + return (await import('node-fetch')).default(...args) } else { throw new Error( 'Invariant: an edge runtime that does not support fetch should not exist' diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts new file mode 100644 index 000000000..491e68812 --- /dev/null +++ b/packages/node/src/lib/http-client.ts @@ -0,0 +1,87 @@ +import type { Analytics } from '../app/analytics-node' +import { NodeEmitter } from '../app/emitter' +import { abortSignalAfterTimeout } from './abort' + +export interface HTTPFetchClientResponse { + ok: boolean + status: number + statusText: string + /** + * Using string, but this is also response type + * "basic" | "cors" | "default" | "error" | "opaque" | "opaqueredirect"; + */ + // type: string +} + +export interface HTTPFetchFn { + ( + url: string | URL, + options?: HTTPClientOptions + ): Promise +} + +export interface HTTPClient { + makeRequest( + _options: HTTPRequestOptions, + emitter: NodeEmitter + ): Promise +} + +interface HTTPRequestOptions { + url: string + method: string + headers: Record + data: Record + timeout: number +} + +export interface HTTPClientOptions { + headers?: Record + body?: string + method?: string + signal?: any // AbortSignal type does not play nicely with node-fetch +} + +/** + * A client that sends http requests. + */ +export interface AnalyticsHTTPClientDELETE { + /** + * Compatible with the fetch API + */ + send( + url: string, + options: HTTPClientOptions + ): Promise +} + +export class FetchHTTPClient implements HTTPClient { + private _fetch: HTTPFetchFn + constructor(fetchFn: HTTPFetchFn | typeof globalThis.fetch) { + this._fetch = fetchFn + } + async makeRequest( + options: HTTPRequestOptions, + analytics: Analytics + ): Promise { + const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) + + const requestInit = { + url: options.url, + method: options.method, + headers: options.headers, + body: JSON.stringify(options.data), + signal: signal, + } + + analytics.emit('http_request', { + url: requestInit.url, + method: requestInit.method, + headers: requestInit.headers, + body: requestInit.body, + }) + const response = await this._fetch(options.url, requestInit) + clearTimeout(timeoutId) + return response + } +} diff --git a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts index beedd411f..c8ee3241c 100644 --- a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts @@ -1,5 +1,3 @@ -const fetcher = jest.fn() -jest.mock('../../../lib/fetch', () => ({ fetch: fetcher })) import { NodeEventFactory } from '../../../app/event-factory' import { createSuccess } from '../../../__tests__/test-helpers/factories' import { createConfiguredNodePlugin } from '../index' @@ -10,10 +8,24 @@ import { bodyPropertyMatchers, assertSegmentApiBody, } from './test-helpers/segment-http-api' +import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-analytics' let emitter: Emitter -const createTestNodePlugin = (props: PublisherProps) => - createConfiguredNodePlugin(props, emitter) +const testClient = new TestFetchClient() +const fetcher = jest.spyOn(testClient, 'send') + +const createTestNodePlugin = (props: Partial = {}) => + createConfiguredNodePlugin( + { + maxRetries: 3, + maxEventsInBatch: 1, + flushInterval: 1000, + writeKey: '', + httpFetchFn: testClient, + ...props, + }, + emitter + ) const validateFetcherInputs = (...contexts: Context[]) => { const [url, request] = fetcher.mock.lastCall @@ -34,6 +46,7 @@ test('alias', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', + httpFetchFn: testClient, }) const event = eventFactory.alias('to', 'from') @@ -46,7 +59,7 @@ test('alias', async () => { validateFetcherInputs(context) const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body) + const body = JSON.parse(request.body!) expect(body.batch).toHaveLength(1) expect(body.batch[0]).toEqual({ @@ -58,12 +71,7 @@ test('alias', async () => { }) test('group', async () => { - const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, - maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', - }) + const { plugin: segmentPlugin } = createTestNodePlugin() const event = eventFactory.group( 'foo-group-id', @@ -81,7 +89,7 @@ test('group', async () => { validateFetcherInputs(context) const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body) + const body = JSON.parse(request.body!) expect(body.batch).toHaveLength(1) expect(body.batch[0]).toEqual({ @@ -96,12 +104,7 @@ test('group', async () => { }) test('identify', async () => { - const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, - maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', - }) + const { plugin: segmentPlugin } = createTestNodePlugin() const event = eventFactory.identify('foo-user-id', { name: 'Chris Radek', @@ -115,7 +118,7 @@ test('identify', async () => { validateFetcherInputs(context) const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body) + const body = JSON.parse(request.body!) expect(body.batch).toHaveLength(1) expect(body.batch[0]).toEqual({ ...bodyPropertyMatchers, @@ -128,12 +131,7 @@ test('identify', async () => { }) test('page', async () => { - const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, - maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', - }) + const { plugin: segmentPlugin } = createTestNodePlugin() const event = eventFactory.page( 'Category', @@ -150,7 +148,7 @@ test('page', async () => { validateFetcherInputs(context) const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body) + const body = JSON.parse(request.body!) expect(body.batch).toHaveLength(1) expect(body.batch[0]).toEqual({ @@ -167,12 +165,7 @@ test('page', async () => { }) test('screen', async () => { - const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, - maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', - }) + const { plugin: segmentPlugin } = createTestNodePlugin() const event = eventFactory.screen( 'Category', @@ -189,7 +182,7 @@ test('screen', async () => { validateFetcherInputs(context) const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body) + const body = JSON.parse(request.body!) expect(body.batch).toHaveLength(1) expect(body.batch[0]).toEqual({ @@ -205,12 +198,7 @@ test('screen', async () => { }) test('track', async () => { - const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, - maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', - }) + const { plugin: segmentPlugin } = createTestNodePlugin() const event = eventFactory.track( 'test event', @@ -226,7 +214,7 @@ test('track', async () => { validateFetcherInputs(context) const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body) + const body = JSON.parse(request.body!) expect(body.batch).toHaveLength(1) expect(body.batch[0]).toEqual({ diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index 149d6ccb3..f2c27c3b3 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -1,5 +1,3 @@ -const fetcher = jest.fn() -jest.mock('../../../lib/fetch', () => ({ fetch: fetcher })) import { Emitter } from '@segment/analytics-core' import { range } from 'lodash' import { createConfiguredNodePlugin } from '..' @@ -10,12 +8,26 @@ import { createSuccess, createError, } from '../../../__tests__/test-helpers/factories' +import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-analytics' import { PublisherProps } from '../publisher' import { assertSegmentApiBody } from './test-helpers/segment-http-api' let emitter: Emitter -const createTestNodePlugin = (props: PublisherProps) => - createConfiguredNodePlugin(props, emitter) +const testClient = new TestFetchClient() +const fetcher = jest.spyOn(testClient, 'send') + +const createTestNodePlugin = (props: Partial = {}) => + createConfiguredNodePlugin( + { + maxEventsInBatch: 1, + httpFetchFn: testClient, + writeKey: '', + flushInterval: 1000, + maxRetries: 3, + ...props, + }, + emitter + ) const validateFetcherInputs = (...contexts: Context[]) => { const [url, request] = fetcher.mock.lastCall @@ -34,8 +46,6 @@ it('supports multiple events in a batch', async () => { const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 3, maxEventsInBatch: 3, - flushInterval: 1000, - writeKey: '', }) // Create 3 events of mixed types to send. @@ -62,8 +72,6 @@ it('supports waiting a max amount of time before sending', async () => { const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 3, maxEventsInBatch: 3, - flushInterval: 1000, - writeKey: '', }) const context = new Context(eventFactory.alias('to', 'from')) @@ -90,8 +98,6 @@ it('sends as soon as batch fills up or max time is reached', async () => { const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 3, maxEventsInBatch: 2, - flushInterval: 1000, - writeKey: '', }) const context = new Context(eventFactory.alias('to', 'from')) @@ -127,7 +133,6 @@ it('sends if batch will exceed max size in bytes when adding event', async () => maxRetries: 3, maxEventsInBatch: 20, flushInterval: 100, - writeKey: '', }) const contexts: Context[] = [] @@ -180,10 +185,7 @@ describe('flushAfterClose', () => { ) const { plugin: segmentPlugin, publisher } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 20, - flushInterval: 1000, - writeKey: '', }) publisher.flushAfterClose(3) @@ -197,10 +199,7 @@ describe('flushAfterClose', () => { it('continues to flush on each event if batch size is 1', async () => { const { plugin: segmentPlugin, publisher } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) publisher.flushAfterClose(3) @@ -213,10 +212,7 @@ describe('flushAfterClose', () => { it('sends immediately once there are no pending items, even if pending events exceeds batch size', async () => { const { plugin: segmentPlugin, publisher } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 3, - flushInterval: 1000, - writeKey: '', }) publisher.flushAfterClose(5) @@ -228,10 +224,7 @@ describe('flushAfterClose', () => { it('works if there are previous items in the batch', async () => { const { plugin: segmentPlugin, publisher } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 7, - flushInterval: 1000, - writeKey: '', }) range(3).forEach(() => segmentPlugin.track(_createTrackCtx())) // should not flush @@ -244,10 +237,7 @@ describe('flushAfterClose', () => { it('works if there are previous items in the batch AND pending items > batch size', async () => { const { plugin: segmentPlugin, publisher } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 7, - flushInterval: 1000, - writeKey: '', }) range(3).forEach(() => segmentPlugin.track(_createTrackCtx())) // should not flush @@ -266,10 +256,7 @@ describe('flushAfterClose', () => { describe('error handling', () => { it('excludes events that are too large', async () => { const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) const context = new Context( @@ -302,10 +289,7 @@ describe('error handling', () => { ) const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) const context = new Context(eventFactory.alias('to', 'from')) @@ -335,8 +319,6 @@ describe('error handling', () => { const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 2, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) const context = new Context(eventFactory.alias('to', 'from')) @@ -365,8 +347,6 @@ describe('error handling', () => { const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 2, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) const context = new Context(eventFactory.alias('my', 'from')) @@ -394,8 +374,6 @@ describe('error handling', () => { const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 0, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) const fn = jest.fn() @@ -417,10 +395,7 @@ describe('error handling', () => { describe('http_request emitter event', () => { it('should emit an http_request object', async () => { const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', }) fetcher.mockReturnValueOnce(createSuccess()) diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index 72f359ce9..afd8df951 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -1,12 +1,11 @@ import { backoff } from '@segment/analytics-core' -import { abortSignalAfterTimeout } from '../../lib/abort' import type { Context } from '../../app/context' import { tryCreateFormattedUrl } from '../../lib/create-url' import { extractPromiseParts } from '../../lib/extract-promise-parts' -import { fetch } from '../../lib/fetch' import { ContextBatch } from './context-batch' import { NodeEmitter } from '../../app/emitter' import { b64encode } from '../../lib/base-64-encode' +import { FetchHTTPClient, HTTPClient, HTTPFetchFn } from '../../lib/http-client' function sleep(timeoutInMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, timeoutInMs)) @@ -28,6 +27,7 @@ export interface PublisherProps { writeKey: string httpRequestTimeout?: number disable?: boolean + httpFetchFn: HTTPFetchFn } /** @@ -46,6 +46,7 @@ export class Publisher { private _httpRequestTimeout: number private _emitter: NodeEmitter private _disable: boolean + private _httpClient: HTTPClient constructor( { host, @@ -55,6 +56,7 @@ export class Publisher { flushInterval, writeKey, httpRequestTimeout, + httpFetchFn, disable, }: PublisherProps, emitter: NodeEmitter @@ -70,6 +72,7 @@ export class Publisher { ) this._httpRequestTimeout = httpRequestTimeout ?? 10000 this._disable = Boolean(disable) + this._httpClient = new FetchHTTPClient(httpFetchFn) } private createBatch(): ContextBatch { @@ -183,7 +186,6 @@ export class Publisher { this._closeAndFlushPendingItemsCount -= batch.length } const events = batch.getEvents() - const payload = JSON.stringify({ batch: events }) const maxAttempts = this._maxRetries + 1 let currentAttempt = 0 @@ -191,36 +193,25 @@ export class Publisher { currentAttempt++ let failureReason: unknown - const [signal, timeoutId] = abortSignalAfterTimeout( - this._httpRequestTimeout - ) try { - const requestInit = { - signal: signal, - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Authorization: `Basic ${this._auth}`, - 'User-Agent': 'analytics-node-next/latest', - }, - body: payload, - } - - this._emitter.emit('http_request', { - url: this._url, - method: requestInit.method, - headers: requestInit.headers, - body: requestInit.body, - }) - if (this._disable) { - clearTimeout(timeoutId) return batch.resolveEvents() } - const response = await fetch(this._url, requestInit) - - clearTimeout(timeoutId) + const response = await this._httpClient.makeRequest( + { + timeout: this._httpRequestTimeout, + url: this._url, + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Basic ${this._auth}`, + 'User-Agent': 'analytics-node-next/latest', + }, + data: { batch: events }, + }, + this._emitter + ) if (response.ok) { // Successfully sent events, so exit! From 4143e98a2ff281a574477cb500f80a817be37148 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Jul 2023 15:34:17 -0500 Subject: [PATCH 02/24] wip --- .../src/__tests__/graceful-shutdown-integration.test.ts | 2 +- packages/node/src/__tests__/integration.test.ts | 2 +- .../src/__tests__/test-helpers/create-test-analytics.ts | 6 +++--- packages/node/src/app/analytics-node.ts | 3 ++- packages/node/src/app/settings.ts | 4 ++-- .../node/src/plugins/segmentio/__tests__/methods.test.ts | 6 +++--- .../src/plugins/segmentio/__tests__/publisher.test.ts | 4 ++-- packages/node/src/plugins/segmentio/publisher.ts | 8 ++++---- 8 files changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts index 86913d384..090c2ea58 100644 --- a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts +++ b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts @@ -14,7 +14,7 @@ const testPlugin: Plugin = { } const testClient = new TestFetchClient() -const sendSpy = jest.spyOn(testClient, 'send') +const sendSpy = jest.spyOn(testClient, 'makeRequest') describe('Ability for users to exit without losing events', () => { let ajs!: Analytics diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index 578fc7bae..adcca2bc3 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -12,7 +12,7 @@ jest.setTimeout(10000) const timestamp = new Date() const testClient = new TestFetchClient() -const sendSpy = jest.spyOn(testClient, 'send') +const sendSpy = jest.spyOn(testClient, 'makeRequest') describe('Settings / Configuration Init', () => { it('throws if no writeKey', () => { diff --git a/packages/node/src/__tests__/test-helpers/create-test-analytics.ts b/packages/node/src/__tests__/test-helpers/create-test-analytics.ts index d3c7203d9..410cb2aa7 100644 --- a/packages/node/src/__tests__/test-helpers/create-test-analytics.ts +++ b/packages/node/src/__tests__/test-helpers/create-test-analytics.ts @@ -1,6 +1,6 @@ import { Analytics } from '../../app/analytics-node' import { AnalyticsSettings } from '../../app/settings' -import { AnalyticsHTTPClientDELETE } from '../../lib/http-client' +import { AnalyticsHTTPClientDELETE, HTTPClient } from '../../lib/http-client' import { createError, createSuccess } from './factories' export const createTestAnalytics = ( @@ -29,14 +29,14 @@ export type TestFetchClientOptions = { /** * Test http client. By default, it will return a successful response. */ -export class TestFetchClient implements AnalyticsHTTPClientDELETE { +export class TestFetchClient implements HTTPClient { private withError?: TestFetchClientOptions['withError'] private response?: TestFetchClientOptions['response'] constructor({ withError, response }: TestFetchClientOptions = {}) { this.withError = withError this.response = response } - send(..._args: Parameters) { + makeRequest() { if (this.response) { return Promise.resolve(this.response) } diff --git a/packages/node/src/app/analytics-node.ts b/packages/node/src/app/analytics-node.ts index a3219886c..29e941e29 100644 --- a/packages/node/src/app/analytics-node.ts +++ b/packages/node/src/app/analytics-node.ts @@ -16,6 +16,7 @@ import { } from './types' import { Context } from './context' import { NodeEventQueue } from './event-queue' +import { FetchHTTPClient } from '../lib/http-client' import { fetch } from '../lib/fetch' export class Analytics extends NodeEmitter implements CoreAnalytics { @@ -52,7 +53,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { httpRequestTimeout: settings.httpRequestTimeout, disable: settings.disable, flushInterval, - httpFetchFn: settings.httpClient ?? fetch, + httpClient: settings.httpClient ?? new FetchHTTPClient(fetch), }, this as NodeEmitter ) diff --git a/packages/node/src/app/settings.ts b/packages/node/src/app/settings.ts index 7b87f4484..39a6c3fbd 100644 --- a/packages/node/src/app/settings.ts +++ b/packages/node/src/app/settings.ts @@ -1,5 +1,5 @@ import { ValidationError } from '@segment/analytics-core' -import { HTTPFetchFn } from '../lib/http-client' +import { HTTPClient } from '../lib/http-client' export interface AnalyticsSettings { /** @@ -38,7 +38,7 @@ export interface AnalyticsSettings { /** * Supply a default http client implementation (such as one supporting proxy). Default: The value of globalThis.fetch, with node-fetch as a fallback. */ - httpClient?: HTTPFetchFn + httpClient?: HTTPClient } export const validateSettings = (settings: AnalyticsSettings) => { diff --git a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts index c8ee3241c..95cfcb309 100644 --- a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts @@ -12,7 +12,7 @@ import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-ana let emitter: Emitter const testClient = new TestFetchClient() -const fetcher = jest.spyOn(testClient, 'send') +const fetcher = jest.spyOn(testClient, 'makeRequest') const createTestNodePlugin = (props: Partial = {}) => createConfiguredNodePlugin( @@ -21,7 +21,7 @@ const createTestNodePlugin = (props: Partial = {}) => maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - httpFetchFn: testClient, + httpClient: testClient, ...props, }, emitter @@ -46,7 +46,7 @@ test('alias', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - httpFetchFn: testClient, + httpClient: testClient, }) const event = eventFactory.alias('to', 'from') diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index f2c27c3b3..eaeafc4c0 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -14,13 +14,13 @@ import { assertSegmentApiBody } from './test-helpers/segment-http-api' let emitter: Emitter const testClient = new TestFetchClient() -const fetcher = jest.spyOn(testClient, 'send') +const fetcher = jest.spyOn(testClient, 'makeRequest') const createTestNodePlugin = (props: Partial = {}) => createConfiguredNodePlugin( { maxEventsInBatch: 1, - httpFetchFn: testClient, + httpClient: testClient, writeKey: '', flushInterval: 1000, maxRetries: 3, diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index afd8df951..97a999fb7 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -5,7 +5,7 @@ import { extractPromiseParts } from '../../lib/extract-promise-parts' import { ContextBatch } from './context-batch' import { NodeEmitter } from '../../app/emitter' import { b64encode } from '../../lib/base-64-encode' -import { FetchHTTPClient, HTTPClient, HTTPFetchFn } from '../../lib/http-client' +import { HTTPClient } from '../../lib/http-client' function sleep(timeoutInMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, timeoutInMs)) @@ -27,7 +27,7 @@ export interface PublisherProps { writeKey: string httpRequestTimeout?: number disable?: boolean - httpFetchFn: HTTPFetchFn + httpClient: HTTPClient } /** @@ -56,7 +56,7 @@ export class Publisher { flushInterval, writeKey, httpRequestTimeout, - httpFetchFn, + httpClient, disable, }: PublisherProps, emitter: NodeEmitter @@ -72,7 +72,7 @@ export class Publisher { ) this._httpRequestTimeout = httpRequestTimeout ?? 10000 this._disable = Boolean(disable) - this._httpClient = new FetchHTTPClient(httpFetchFn) + this._httpClient = httpClient } private createBatch(): ContextBatch { From 79c683370298d7d1f587cd2846dba21e0d521b2e Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Jul 2023 18:02:07 -0500 Subject: [PATCH 03/24] wip --- .../src/__tests__/disable.integration.test.ts | 10 +-- .../graceful-shutdown-integration.test.ts | 17 ++--- .../node/src/__tests__/integration.test.ts | 4 +- .../assert-shape/http-request-event.ts | 2 +- .../test-helpers/create-test-analytics.ts | 19 +++--- packages/node/src/app/analytics-node.ts | 5 +- packages/node/src/app/emitter.ts | 2 +- packages/node/src/app/settings.ts | 8 ++- packages/node/src/index.ts | 3 +- packages/node/src/lib/__tests__/abort.test.ts | 1 + packages/node/src/lib/http-client.ts | 46 ++++---------- .../segmentio/__tests__/methods.test.ts | 62 +++++++++---------- .../segmentio/__tests__/publisher.test.ts | 8 +-- .../test-helpers/segment-http-api.ts | 25 ++++---- .../__tests__/test-helpers/validate-http.ts | 0 .../node/src/plugins/segmentio/publisher.ts | 34 +++++----- 16 files changed, 113 insertions(+), 133 deletions(-) create mode 100644 packages/node/src/plugins/segmentio/__tests__/test-helpers/validate-http.ts diff --git a/packages/node/src/__tests__/disable.integration.test.ts b/packages/node/src/__tests__/disable.integration.test.ts index 3a8b09c8b..c5d1dba41 100644 --- a/packages/node/src/__tests__/disable.integration.test.ts +++ b/packages/node/src/__tests__/disable.integration.test.ts @@ -5,9 +5,9 @@ import { describe('disable', () => { const httpClient = new TestFetchClient() - const mockSend = jest.spyOn(httpClient, 'send') + const makeReqSpy = jest.spyOn(httpClient, 'makeRequest') - it('should dispatch callbacks and emit an http request, even if disabled', async () => { + it('should not emit an http request if disabled', async () => { const analytics = createTestAnalytics({ disable: true, }) @@ -16,7 +16,7 @@ describe('disable', () => { await new Promise((resolve) => analytics.track({ anonymousId: 'foo', event: 'bar' }, resolve) ) - expect(emitterCb).toBeCalledTimes(1) + expect(emitterCb).not.toBeCalled() }) it('should call .send if disabled is false', async () => { @@ -27,7 +27,7 @@ describe('disable', () => { await new Promise((resolve) => analytics.track({ anonymousId: 'foo', event: 'bar' }, resolve) ) - expect(mockSend).toBeCalledTimes(1) + expect(makeReqSpy).toBeCalledTimes(1) }) it('should not call .send if disabled is true', async () => { const analytics = createTestAnalytics({ @@ -37,6 +37,6 @@ describe('disable', () => { await new Promise((resolve) => analytics.track({ anonymousId: 'foo', event: 'bar' }, resolve) ) - expect(mockSend).not.toBeCalled() + expect(makeReqSpy).not.toBeCalled() }) }) diff --git a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts index 090c2ea58..e82889c99 100644 --- a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts +++ b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts @@ -13,11 +13,12 @@ const testPlugin: Plugin = { isLoaded: () => true, } -const testClient = new TestFetchClient() -const sendSpy = jest.spyOn(testClient, 'makeRequest') +let testClient: TestFetchClient describe('Ability for users to exit without losing events', () => { let ajs!: Analytics + testClient = new TestFetchClient() + const makeReqSpy = jest.spyOn(testClient, 'makeRequest') beforeEach(async () => { ajs = new Analytics({ writeKey: 'abc123', @@ -27,11 +28,11 @@ describe('Ability for users to exit without losing events', () => { }) const _helpers = { getFetchCalls: () => - sendSpy.mock.calls.map(([url, request]) => ({ + makeReqSpy.mock.calls.map(([{ url, method, data, headers }]) => ({ url, - method: request.method, - headers: request.headers, - body: JSON.parse(request.body!), + method, + headers, + data, })), makeTrackCall: (analytics = ajs, cb?: (...args: any[]) => void) => { analytics.track({ userId: 'foo', event: 'Thing Updated' }, cb) @@ -205,7 +206,7 @@ describe('Ability for users to exit without losing events', () => { expect(elapsedTime).toBeLessThan(100) const calls = _helpers.getFetchCalls() expect(calls.length).toBe(1) - expect(calls[0].body.batch.length).toBe(2) + expect(calls[0].data.batch.length).toBe(2) }) test('should wait to flush if close is called and an event has not made it to the segment.io plugin yet', async () => { @@ -237,7 +238,7 @@ describe('Ability for users to exit without losing events', () => { expect(elapsedTime).toBeLessThan(TRACK_DELAY * 2) const calls = _helpers.getFetchCalls() expect(calls.length).toBe(1) - expect(calls[0].body.batch.length).toBe(2) + expect(calls[0].data.batch.length).toBe(2) }) }) }) diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index adcca2bc3..463a5fc81 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -12,7 +12,7 @@ jest.setTimeout(10000) const timestamp = new Date() const testClient = new TestFetchClient() -const sendSpy = jest.spyOn(testClient, 'makeRequest') +const makeReqSpy = jest.spyOn(testClient, 'makeRequest') describe('Settings / Configuration Init', () => { it('throws if no writeKey', () => { @@ -32,7 +32,7 @@ describe('Settings / Configuration Init', () => { const track = resolveCtx(analytics, 'track') analytics.track({ event: 'foo', userId: 'sup' }) await track - expect(sendSpy.mock.calls[0][0]).toBe('http://foo.com/bar') + expect(makeReqSpy.mock.calls[0][0].url).toBe('http://foo.com/bar') }) it('throws if host / path is bad', async () => { diff --git a/packages/node/src/__tests__/test-helpers/assert-shape/http-request-event.ts b/packages/node/src/__tests__/test-helpers/assert-shape/http-request-event.ts index 7c0b733ce..684a32dc2 100644 --- a/packages/node/src/__tests__/test-helpers/assert-shape/http-request-event.ts +++ b/packages/node/src/__tests__/test-helpers/assert-shape/http-request-event.ts @@ -4,7 +4,7 @@ type HttpRequestEmitterEvent = NodeEmitterEvents['http_request'][0] export const assertHttpRequestEmittedEvent = ( event: HttpRequestEmitterEvent ) => { - const body = JSON.parse(event.body) + const body = event.body expect(Array.isArray(body.batch)).toBeTruthy() expect(body.batch.length).toBe(1) expect(typeof event.headers).toBe('object') diff --git a/packages/node/src/__tests__/test-helpers/create-test-analytics.ts b/packages/node/src/__tests__/test-helpers/create-test-analytics.ts index 410cb2aa7..6d91535a2 100644 --- a/packages/node/src/__tests__/test-helpers/create-test-analytics.ts +++ b/packages/node/src/__tests__/test-helpers/create-test-analytics.ts @@ -1,6 +1,6 @@ import { Analytics } from '../../app/analytics-node' import { AnalyticsSettings } from '../../app/settings' -import { AnalyticsHTTPClientDELETE, HTTPClient } from '../../lib/http-client' +import { FetchHTTPClient, HTTPFetchFn } from '../../lib/http-client' import { createError, createSuccess } from './factories' export const createTestAnalytics = ( @@ -29,17 +29,14 @@ export type TestFetchClientOptions = { /** * Test http client. By default, it will return a successful response. */ -export class TestFetchClient implements HTTPClient { - private withError?: TestFetchClientOptions['withError'] - private response?: TestFetchClientOptions['response'] +export class TestFetchClient extends FetchHTTPClient { constructor({ withError, response }: TestFetchClientOptions = {}) { - this.withError = withError - this.response = response - } - makeRequest() { - if (this.response) { - return Promise.resolve(this.response) + const _mockFetch: HTTPFetchFn = (..._args) => { + if (response) { + return Promise.resolve(response) + } + return Promise.resolve(withError ? createError() : createSuccess()) } - return Promise.resolve(this.withError ? createError() : createSuccess()) + super(_mockFetch) } } diff --git a/packages/node/src/app/analytics-node.ts b/packages/node/src/app/analytics-node.ts index 29e941e29..cd0742793 100644 --- a/packages/node/src/app/analytics-node.ts +++ b/packages/node/src/app/analytics-node.ts @@ -53,7 +53,10 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { httpRequestTimeout: settings.httpRequestTimeout, disable: settings.disable, flushInterval, - httpClient: settings.httpClient ?? new FetchHTTPClient(fetch), + httpClient: + typeof settings.httpClient === 'function' + ? new FetchHTTPClient(settings.httpClient) + : settings.httpClient ?? new FetchHTTPClient(fetch), }, this as NodeEmitter ) diff --git a/packages/node/src/app/emitter.ts b/packages/node/src/app/emitter.ts index f68f9d19a..4d935a09b 100644 --- a/packages/node/src/app/emitter.ts +++ b/packages/node/src/app/emitter.ts @@ -14,7 +14,7 @@ export type NodeEmitterEvents = CoreEmitterContract & { url: string method: string headers: Record - body: string + body: any } ] drained: [] diff --git a/packages/node/src/app/settings.ts b/packages/node/src/app/settings.ts index 39a6c3fbd..e66bc38ac 100644 --- a/packages/node/src/app/settings.ts +++ b/packages/node/src/app/settings.ts @@ -1,5 +1,5 @@ import { ValidationError } from '@segment/analytics-core' -import { HTTPClient } from '../lib/http-client' +import { HTTPClient, HTTPFetchFn } from '../lib/http-client' export interface AnalyticsSettings { /** @@ -36,9 +36,11 @@ export interface AnalyticsSettings { */ disable?: boolean /** - * Supply a default http client implementation (such as one supporting proxy). Default: The value of globalThis.fetch, with node-fetch as a fallback. + * Supply a default http client implementation (such as one supporting proxy). + * Accepts either an HTTPClient instance or a fetch function. + * Default: an HTTP client that uses globalThis.fetch, with node-fetch as a fallback. */ - httpClient?: HTTPClient + httpClient?: HTTPFetchFn | HTTPClient } export const validateSettings = (settings: AnalyticsSettings) => { diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 9701b6365..a4212fee1 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -2,8 +2,7 @@ export { Analytics } from './app/analytics-node' export { Context } from './app/context' export type { FetchHTTPClient, - AnalyticsHTTPClientDELETE, - HTTPClientOptions, + FetchHTTPClientOptions, HTTPFetchFn, HTTPFetchClientResponse, } from './lib/http-client' diff --git a/packages/node/src/lib/__tests__/abort.test.ts b/packages/node/src/lib/__tests__/abort.test.ts index 82a4336f2..ecc4af764 100644 --- a/packages/node/src/lib/__tests__/abort.test.ts +++ b/packages/node/src/lib/__tests__/abort.test.ts @@ -1,5 +1,6 @@ import { abortSignalAfterTimeout } from '../abort' import nock from 'nock' +import { fetch } from '../fetch' import { sleep } from '@segment/analytics-core' describe(abortSignalAfterTimeout, () => { diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 491e68812..1e0ea7f85 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -1,5 +1,3 @@ -import type { Analytics } from '../app/analytics-node' -import { NodeEmitter } from '../app/emitter' import { abortSignalAfterTimeout } from './abort' export interface HTTPFetchClientResponse { @@ -13,21 +11,21 @@ export interface HTTPFetchClientResponse { // type: string } +/** + * This interface is meant to be compatible with different fetch implementations. + */ export interface HTTPFetchFn { ( - url: string | URL, - options?: HTTPClientOptions + url: string, + options: FetchHTTPClientOptions ): Promise } export interface HTTPClient { - makeRequest( - _options: HTTPRequestOptions, - emitter: NodeEmitter - ): Promise + makeRequest(_options: HTTPRequestOptions): Promise } -interface HTTPRequestOptions { +export interface HTTPRequestOptions { url: string method: string headers: Record @@ -35,34 +33,20 @@ interface HTTPRequestOptions { timeout: number } -export interface HTTPClientOptions { +export interface FetchHTTPClientOptions { headers?: Record body?: string method?: string signal?: any // AbortSignal type does not play nicely with node-fetch } -/** - * A client that sends http requests. - */ -export interface AnalyticsHTTPClientDELETE { - /** - * Compatible with the fetch API - */ - send( - url: string, - options: HTTPClientOptions - ): Promise -} - export class FetchHTTPClient implements HTTPClient { private _fetch: HTTPFetchFn constructor(fetchFn: HTTPFetchFn | typeof globalThis.fetch) { this._fetch = fetchFn } async makeRequest( - options: HTTPRequestOptions, - analytics: Analytics + options: HTTPRequestOptions ): Promise { const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) @@ -74,14 +58,8 @@ export class FetchHTTPClient implements HTTPClient { signal: signal, } - analytics.emit('http_request', { - url: requestInit.url, - method: requestInit.method, - headers: requestInit.headers, - body: requestInit.body, - }) - const response = await this._fetch(options.url, requestInit) - clearTimeout(timeoutId) - return response + return this._fetch(options.url, requestInit).finally(() => + clearTimeout(timeoutId) + ) } } diff --git a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts index 95cfcb309..c188a6752 100644 --- a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts @@ -6,7 +6,7 @@ import { Context } from '../../../app/context' import { Emitter } from '@segment/analytics-core' import { bodyPropertyMatchers, - assertSegmentApiBody, + assertHTTPRequestOptions, } from './test-helpers/segment-http-api' import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-analytics' @@ -28,8 +28,8 @@ const createTestNodePlugin = (props: Partial = {}) => ) const validateFetcherInputs = (...contexts: Context[]) => { - const [url, request] = fetcher.mock.lastCall - return assertSegmentApiBody(url, request, contexts) + const [request] = fetcher.mock.lastCall + return assertHTTPRequestOptions(request, contexts) } const eventFactory = new NodeEventFactory() @@ -41,13 +41,7 @@ beforeEach(() => { }) test('alias', async () => { - const { plugin: segmentPlugin } = createTestNodePlugin({ - maxRetries: 3, - maxEventsInBatch: 1, - flushInterval: 1000, - writeKey: '', - httpClient: testClient, - }) + const { plugin: segmentPlugin } = createTestNodePlugin() const event = eventFactory.alias('to', 'from') const context = new Context(event) @@ -58,11 +52,11 @@ test('alias', async () => { expect(fetcher).toHaveBeenCalledTimes(1) validateFetcherInputs(context) - const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body!) + const [request] = fetcher.mock.lastCall + const data = request.data - expect(body.batch).toHaveLength(1) - expect(body.batch[0]).toEqual({ + expect(data.batch).toHaveLength(1) + expect(data.batch[0]).toEqual({ ...bodyPropertyMatchers, type: 'alias', previousId: 'from', @@ -88,11 +82,11 @@ test('group', async () => { expect(fetcher).toHaveBeenCalledTimes(1) validateFetcherInputs(context) - const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body!) + const [request] = fetcher.mock.lastCall + const data = request.data - expect(body.batch).toHaveLength(1) - expect(body.batch[0]).toEqual({ + expect(data.batch).toHaveLength(1) + expect(data.batch[0]).toEqual({ ...bodyPropertyMatchers, traits: { name: 'libraries', @@ -117,10 +111,10 @@ test('identify', async () => { expect(fetcher).toHaveBeenCalledTimes(1) validateFetcherInputs(context) - const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body!) - expect(body.batch).toHaveLength(1) - expect(body.batch[0]).toEqual({ + const [request] = fetcher.mock.lastCall + const data = request.data + expect(data.batch).toHaveLength(1) + expect(data.batch[0]).toEqual({ ...bodyPropertyMatchers, traits: { name: 'Chris Radek', @@ -147,11 +141,11 @@ test('page', async () => { expect(fetcher).toHaveBeenCalledTimes(1) validateFetcherInputs(context) - const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body!) + const [request] = fetcher.mock.lastCall + const data = request.data - expect(body.batch).toHaveLength(1) - expect(body.batch[0]).toEqual({ + expect(data.batch).toHaveLength(1) + expect(data.batch[0]).toEqual({ ...bodyPropertyMatchers, type: 'page', userId: 'foo-user-id', @@ -181,11 +175,11 @@ test('screen', async () => { expect(fetcher).toHaveBeenCalledTimes(1) validateFetcherInputs(context) - const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body!) + const [request] = fetcher.mock.lastCall + const data = request.data - expect(body.batch).toHaveLength(1) - expect(body.batch[0]).toEqual({ + expect(data.batch).toHaveLength(1) + expect(data.batch[0]).toEqual({ ...bodyPropertyMatchers, type: 'screen', userId: 'foo-user-id', @@ -213,11 +207,11 @@ test('track', async () => { expect(fetcher).toHaveBeenCalledTimes(1) validateFetcherInputs(context) - const [, request] = fetcher.mock.lastCall - const body = JSON.parse(request.body!) + const [request] = fetcher.mock.lastCall + const data = request.data - expect(body.batch).toHaveLength(1) - expect(body.batch[0]).toEqual({ + expect(data.batch).toHaveLength(1) + expect(data.batch[0]).toEqual({ ...bodyPropertyMatchers, type: 'track', event: 'test event', diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index eaeafc4c0..e2fec333b 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -10,7 +10,7 @@ import { } from '../../../__tests__/test-helpers/factories' import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-analytics' import { PublisherProps } from '../publisher' -import { assertSegmentApiBody } from './test-helpers/segment-http-api' +import { assertHTTPRequestOptions } from './test-helpers/segment-http-api' let emitter: Emitter const testClient = new TestFetchClient() @@ -30,8 +30,8 @@ const createTestNodePlugin = (props: Partial = {}) => ) const validateFetcherInputs = (...contexts: Context[]) => { - const [url, request] = fetcher.mock.lastCall - return assertSegmentApiBody(url, request, contexts) + const [request] = fetcher.mock.lastCall + return assertHTTPRequestOptions(request, contexts) } const eventFactory = new NodeEventFactory() @@ -308,7 +308,7 @@ describe('error handling', () => { `) }) - it('retries non-400 errors', async () => { + it.only('retries non-400 errors', async () => { // Jest kept timing out when using fake timers despite advancing time. jest.useRealTimers() diff --git a/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts b/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts index 97847ccb5..726aebcee 100644 --- a/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts +++ b/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts @@ -1,10 +1,9 @@ import { Context } from '../../../../app/context' +import { HTTPRequestOptions } from '../../../../lib/http-client' export const bodyPropertyMatchers = { messageId: expect.stringMatching(/^node-next-\d*-\w*-\w*-\w*-\w*-\w*/), - timestamp: expect.stringMatching( - /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/ - ), + timestamp: expect.any(Date), _metadata: expect.any(Object), context: { library: { @@ -15,15 +14,13 @@ export const bodyPropertyMatchers = { integrations: {}, } -export function assertSegmentApiBody( - url: string, - request: RequestInit, +export function assertHTTPRequestOptions( + { data, headers, method, url }: HTTPRequestOptions, contexts: Context[] ) { - const body = JSON.parse(request.body as string) expect(url).toBe('https://api.segment.io/v1/batch') - expect(request.method).toBe('POST') - expect(request.headers).toMatchInlineSnapshot(` + expect(method).toBe('POST') + expect(headers).toMatchInlineSnapshot(` Object { "Authorization": "Basic Og==", "Content-Type": "application/json", @@ -31,11 +28,13 @@ export function assertSegmentApiBody( } `) - expect(body.batch).toHaveLength(contexts.length) - for (let i = 0; i < contexts.length; i++) { - expect(body.batch[i]).toEqual({ - ...contexts[i].event, + expect(data.batch).toHaveLength(contexts.length) + let idx = 0 + for (const context of contexts) { + expect(data.batch[idx]).toEqual({ ...bodyPropertyMatchers, + ...context.event, }) + idx += 1 } } diff --git a/packages/node/src/plugins/segmentio/__tests__/test-helpers/validate-http.ts b/packages/node/src/plugins/segmentio/__tests__/test-helpers/validate-http.ts new file mode 100644 index 000000000..e69de29bb diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index 97a999fb7..a7b466608 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -5,7 +5,7 @@ import { extractPromiseParts } from '../../lib/extract-promise-parts' import { ContextBatch } from './context-batch' import { NodeEmitter } from '../../app/emitter' import { b64encode } from '../../lib/base-64-encode' -import { HTTPClient } from '../../lib/http-client' +import { HTTPClient, HTTPRequestOptions } from '../../lib/http-client' function sleep(timeoutInMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, timeoutInMs)) @@ -198,20 +198,26 @@ export class Publisher { return batch.resolveEvents() } - const response = await this._httpClient.makeRequest( - { - timeout: this._httpRequestTimeout, - url: this._url, - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Authorization: `Basic ${this._auth}`, - 'User-Agent': 'analytics-node-next/latest', - }, - data: { batch: events }, + const request: HTTPRequestOptions = { + timeout: this._httpRequestTimeout, + url: this._url, + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Basic ${this._auth}`, + 'User-Agent': 'analytics-node-next/latest', }, - this._emitter - ) + data: { batch: events }, + } + + this._emitter.emit('http_request', { + body: request.data, + method: request.method, + url: request.url, + headers: request.headers, + }) + + const response = await this._httpClient.makeRequest(request) if (response.ok) { // Successfully sent events, so exit! From a6a0d951f02bbbdf326f115f362fc280f78e04d2 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Jul 2023 18:14:05 -0500 Subject: [PATCH 04/24] wip --- packages/node/src/__tests__/typedef-tests.ts | 33 +++++++++++++------- packages/node/src/index.ts | 7 +++-- packages/node/src/lib/http-client.ts | 13 +++----- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/packages/node/src/__tests__/typedef-tests.ts b/packages/node/src/__tests__/typedef-tests.ts index c7d414b9c..45ef98f55 100644 --- a/packages/node/src/__tests__/typedef-tests.ts +++ b/packages/node/src/__tests__/typedef-tests.ts @@ -1,10 +1,12 @@ +/* eslint-disable @typescript-eslint/no-var-requires */ import { Analytics, Context, Plugin, UserTraits, GroupTraits, - AnalyticsHTTPClientDELETE, + HTTPClient, + FetchHTTPClient, } from '../' /** @@ -21,19 +23,28 @@ export default { analytics.VERSION = 'foo' }, - 'httpClient setting should be compatible with standard fetch and node-fetch interface': + 'httpClient setting should be compatible with standard fetch and node-fetch interface, as well as functions': () => { - const nodeFetchClient: AnalyticsHTTPClientDELETE = { - send: require('node-fetch'), - } - new Analytics({ writeKey: 'foo', httpClient: nodeFetchClient }) - - const standardFetchClient: AnalyticsHTTPClientDELETE = { - send: fetch, - } - new Analytics({ writeKey: 'foo', httpClient: standardFetchClient }) + new Analytics({ writeKey: 'foo', httpClient: require('node-fetch') }) + new Analytics({ writeKey: 'foo', httpClient: globalThis.fetch }) }, + 'Analytics should accept an entire HTTP Client': () => { + class CustomClient implements HTTPClient { + makeRequest = () => Promise.resolve({} as Response) + } + + new Analytics({ + writeKey: 'foo', + httpClient: new CustomClient(), + }) + + new Analytics({ + writeKey: 'foo', + httpClient: new FetchHTTPClient(globalThis.fetch), + }) + }, + 'track/id/pg/screen/grp calls should require either userId or anonymousId': () => { const analytics = new Analytics({ writeKey: 'abc' }) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index a4212fee1..ca2a91172 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -1,11 +1,14 @@ export { Analytics } from './app/analytics-node' export { Context } from './app/context' -export type { +export { + HTTPClient, FetchHTTPClient, FetchHTTPClientOptions, - HTTPFetchFn, HTTPFetchClientResponse, + HTTPFetchFn, + HTTPRequestOptions, } from './lib/http-client' + export type { Plugin, GroupTraits, diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 1e0ea7f85..6bdd0a82a 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -4,11 +4,6 @@ export interface HTTPFetchClientResponse { ok: boolean status: number statusText: string - /** - * Using string, but this is also response type - * "basic" | "cors" | "default" | "error" | "opaque" | "opaqueredirect"; - */ - // type: string } /** @@ -21,10 +16,6 @@ export interface HTTPFetchFn { ): Promise } -export interface HTTPClient { - makeRequest(_options: HTTPRequestOptions): Promise -} - export interface HTTPRequestOptions { url: string method: string @@ -40,6 +31,10 @@ export interface FetchHTTPClientOptions { signal?: any // AbortSignal type does not play nicely with node-fetch } +export interface HTTPClient { + makeRequest(_options: HTTPRequestOptions): Promise +} + export class FetchHTTPClient implements HTTPClient { private _fetch: HTTPFetchFn constructor(fetchFn: HTTPFetchFn | typeof globalThis.fetch) { From 5509a84ac711b02e5c97776c04506fe1265ce82f Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Jul 2023 18:30:14 -0500 Subject: [PATCH 05/24] wip --- packages/node/src/plugins/segmentio/__tests__/publisher.test.ts | 2 +- .../segmentio/__tests__/test-helpers/segment-http-api.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index e2fec333b..4602c9448 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -308,7 +308,7 @@ describe('error handling', () => { `) }) - it.only('retries non-400 errors', async () => { + it('retries non-400 errors', async () => { // Jest kept timing out when using fake timers despite advancing time. jest.useRealTimers() diff --git a/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts b/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts index 726aebcee..9e2ee48f2 100644 --- a/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts +++ b/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts @@ -32,8 +32,8 @@ export function assertHTTPRequestOptions( let idx = 0 for (const context of contexts) { expect(data.batch[idx]).toEqual({ - ...bodyPropertyMatchers, ...context.event, + ...bodyPropertyMatchers, }) idx += 1 } From 22cb6e559a993cdbd578610ff23ce54a2f6e597a Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Thu, 6 Jul 2023 19:51:44 -0500 Subject: [PATCH 06/24] wip --- .../src/plugins/segmentio/__tests__/test-helpers/validate-http.ts | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 packages/node/src/plugins/segmentio/__tests__/test-helpers/validate-http.ts diff --git a/packages/node/src/plugins/segmentio/__tests__/test-helpers/validate-http.ts b/packages/node/src/plugins/segmentio/__tests__/test-helpers/validate-http.ts deleted file mode 100644 index e69de29bb..000000000 From 02fcf9c3396b17ade4b52842154d19baa64615c8 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 01:51:49 -0500 Subject: [PATCH 07/24] Update emitter.ts --- packages/node/src/app/emitter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/app/emitter.ts b/packages/node/src/app/emitter.ts index 4d935a09b..344b0eda1 100644 --- a/packages/node/src/app/emitter.ts +++ b/packages/node/src/app/emitter.ts @@ -14,7 +14,7 @@ export type NodeEmitterEvents = CoreEmitterContract & { url: string method: string headers: Record - body: any + body: Record } ] drained: [] From 7c59f8ffdedf23c9e85dbf033d658cd54fa74ff2 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 02:46:59 -0500 Subject: [PATCH 08/24] Update emitter.ts --- packages/node/src/app/emitter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/app/emitter.ts b/packages/node/src/app/emitter.ts index 344b0eda1..b1d9abf2f 100644 --- a/packages/node/src/app/emitter.ts +++ b/packages/node/src/app/emitter.ts @@ -14,7 +14,7 @@ export type NodeEmitterEvents = CoreEmitterContract & { url: string method: string headers: Record - body: Record + body: Record } ] drained: [] From cac9d8194df018e2d982ad23f45ac4fb01dfd11c Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 12:05:57 -0500 Subject: [PATCH 09/24] wip --- packages/node/src/index.ts | 2 +- packages/node/src/lib/http-client.ts | 25 +++++++++++++++++-------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index ca2a91172..2bf81332a 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -3,7 +3,7 @@ export { Context } from './app/context' export { HTTPClient, FetchHTTPClient, - FetchHTTPClientOptions, + HTTPFetchRequestInit, HTTPFetchClientResponse, HTTPFetchFn, HTTPRequestOptions, diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 6bdd0a82a..bf9c42496 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -8,14 +8,30 @@ export interface HTTPFetchClientResponse { /** * This interface is meant to be compatible with different fetch implementations. + * @link https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API */ export interface HTTPFetchFn { ( url: string, - options: FetchHTTPClientOptions + requestInit: HTTPFetchRequestInit ): Promise } +/** + * This interface is meant to conform to the standard Fetch API RequestInit interface. + * @link https://developer.mozilla.org/en-US/docs/Web/API/Request + */ +export interface HTTPFetchRequestInit { + headers?: Record + body?: string + method?: string + signal?: any // AbortSignal type does not play nicely with node-fetch +} + +/** + * This interface is meant to be a generic interface for making HTTP requests. + * While it may overlap with fetch's Request interface, it is not coupled to it. + */ export interface HTTPRequestOptions { url: string method: string @@ -24,13 +40,6 @@ export interface HTTPRequestOptions { timeout: number } -export interface FetchHTTPClientOptions { - headers?: Record - body?: string - method?: string - signal?: any // AbortSignal type does not play nicely with node-fetch -} - export interface HTTPClient { makeRequest(_options: HTTPRequestOptions): Promise } From 3d8d734a19a78385031c4412e20c2785e66b4c64 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 12:11:17 -0500 Subject: [PATCH 10/24] wip --- packages/node/src/lib/http-client.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index bf9c42496..492cc72fb 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -33,10 +33,29 @@ export interface HTTPFetchRequestInit { * While it may overlap with fetch's Request interface, it is not coupled to it. */ export interface HTTPRequestOptions { + /** + * URL to be used for the request + * @example 'https://api.segment.io/v1/batch' + */ url: string + /** + * HTTP method to be used for the request + * @example 'POST' + **/ method: string + /** + * Headers to be sent with the request + */ headers: Record + /** + * JSON data to be sent with the request (will be stringified) + * @example { "batch": [ ... ]} + */ data: Record + /** + * Timeout in milliseconds + * @example 10000 + */ timeout: number } @@ -46,7 +65,7 @@ export interface HTTPClient { export class FetchHTTPClient implements HTTPClient { private _fetch: HTTPFetchFn - constructor(fetchFn: HTTPFetchFn | typeof globalThis.fetch) { + constructor(fetchFn: HTTPFetchFn) { this._fetch = fetchFn } async makeRequest( From 230bc9ef1dfef5d27156366f2aa544b1c2e606c3 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 12:14:13 -0500 Subject: [PATCH 11/24] wip --- packages/node/src/index.ts | 4 ++-- packages/node/src/lib/http-client.ts | 31 ++++++++++++++-------------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 2bf81332a..32f71297e 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -3,8 +3,8 @@ export { Context } from './app/context' export { HTTPClient, FetchHTTPClient, - HTTPFetchRequestInit, - HTTPFetchClientResponse, + HTTPFetchRequest, + HTTPFetchResponse, HTTPFetchFn, HTTPRequestOptions, } from './lib/http-client' diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 492cc72fb..18125c98b 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -1,33 +1,34 @@ import { abortSignalAfterTimeout } from './abort' -export interface HTTPFetchClientResponse { - ok: boolean - status: number - statusText: string -} - /** * This interface is meant to be compatible with different fetch implementations. * @link https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API */ export interface HTTPFetchFn { - ( - url: string, - requestInit: HTTPFetchRequestInit - ): Promise + (url: string, requestInit: HTTPFetchRequest): Promise } /** - * This interface is meant to conform to the standard Fetch API RequestInit interface. + * This interface is meant to he compatible with the Request interface. * @link https://developer.mozilla.org/en-US/docs/Web/API/Request */ -export interface HTTPFetchRequestInit { +export interface HTTPFetchRequest { headers?: Record body?: string method?: string signal?: any // AbortSignal type does not play nicely with node-fetch } +/** + * This interface is meant to conform to the Fetch API Response interface. + * @link https://developer.mozilla.org/en-US/docs/Web/API/Response + */ +export interface HTTPFetchResponse { + ok: boolean + status: number + statusText: string +} + /** * This interface is meant to be a generic interface for making HTTP requests. * While it may overlap with fetch's Request interface, it is not coupled to it. @@ -60,7 +61,7 @@ export interface HTTPRequestOptions { } export interface HTTPClient { - makeRequest(_options: HTTPRequestOptions): Promise + makeRequest(_options: HTTPRequestOptions): Promise } export class FetchHTTPClient implements HTTPClient { @@ -68,9 +69,7 @@ export class FetchHTTPClient implements HTTPClient { constructor(fetchFn: HTTPFetchFn) { this._fetch = fetchFn } - async makeRequest( - options: HTTPRequestOptions - ): Promise { + async makeRequest(options: HTTPRequestOptions): Promise { const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) const requestInit = { From 0af7e85612420be231e993c68f387fcde284f771 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 12:22:06 -0500 Subject: [PATCH 12/24] wip --- packages/node/src/__tests__/typedef-tests.ts | 5 +++++ packages/node/src/app/analytics-node.ts | 3 +-- packages/node/src/lib/http-client.ts | 14 +++++++++++--- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/node/src/__tests__/typedef-tests.ts b/packages/node/src/__tests__/typedef-tests.ts index 45ef98f55..4f3aedf8c 100644 --- a/packages/node/src/__tests__/typedef-tests.ts +++ b/packages/node/src/__tests__/typedef-tests.ts @@ -43,6 +43,11 @@ export default { writeKey: 'foo', httpClient: new FetchHTTPClient(globalThis.fetch), }) + + new Analytics({ + writeKey: 'foo', + httpClient: new FetchHTTPClient(), + }) }, 'track/id/pg/screen/grp calls should require either userId or anonymousId': diff --git a/packages/node/src/app/analytics-node.ts b/packages/node/src/app/analytics-node.ts index cd0742793..35edf8ffb 100644 --- a/packages/node/src/app/analytics-node.ts +++ b/packages/node/src/app/analytics-node.ts @@ -17,7 +17,6 @@ import { import { Context } from './context' import { NodeEventQueue } from './event-queue' import { FetchHTTPClient } from '../lib/http-client' -import { fetch } from '../lib/fetch' export class Analytics extends NodeEmitter implements CoreAnalytics { private readonly _eventFactory: NodeEventFactory @@ -56,7 +55,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { httpClient: typeof settings.httpClient === 'function' ? new FetchHTTPClient(settings.httpClient) - : settings.httpClient ?? new FetchHTTPClient(fetch), + : settings.httpClient ?? new FetchHTTPClient(), }, this as NodeEmitter ) diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 18125c98b..2fc550850 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -1,7 +1,9 @@ import { abortSignalAfterTimeout } from './abort' +import { fetch as defaultFetch } from './fetch' /** - * This interface is meant to be compatible with different fetch implementations. + * This interface is meant to be compatible with different fetch implementations (node and browser). + * Using the ambient fetch type is not possible because the AbortSignal type is not compatible with node-fetch. * @link https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API */ export interface HTTPFetchFn { @@ -60,14 +62,20 @@ export interface HTTPRequestOptions { timeout: number } +/** + * HTTP client interface for making requests.cccccbenidtinulcgklueududltvedrjgeligdefjkfb + */ export interface HTTPClient { makeRequest(_options: HTTPRequestOptions): Promise } +/** + * Default HTTP client implementation using fetch. + */ export class FetchHTTPClient implements HTTPClient { private _fetch: HTTPFetchFn - constructor(fetchFn: HTTPFetchFn) { - this._fetch = fetchFn + constructor(fetchFn?: HTTPFetchFn) { + this._fetch = fetchFn ?? defaultFetch } async makeRequest(options: HTTPRequestOptions): Promise { const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) From 4e3dbb1297d0d7b24c21537204c9eaa462461284 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 13:53:32 -0500 Subject: [PATCH 13/24] wip --- .../__tests__/http-client.integration.test.ts | 44 +++++++++++++++++++ .../test-helpers/assert-shape/index.ts | 1 + .../assert-shape}/segment-http-api.ts | 11 +++-- .../segmentio/__tests__/methods.test.ts | 16 +++---- .../segmentio/__tests__/publisher.test.ts | 2 +- 5 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 packages/node/src/__tests__/http-client.integration.test.ts rename packages/node/src/{plugins/segmentio/__tests__/test-helpers => __tests__/test-helpers/assert-shape}/segment-http-api.ts (74%) diff --git a/packages/node/src/__tests__/http-client.integration.test.ts b/packages/node/src/__tests__/http-client.integration.test.ts new file mode 100644 index 000000000..b6b50ccc7 --- /dev/null +++ b/packages/node/src/__tests__/http-client.integration.test.ts @@ -0,0 +1,44 @@ +import { HTTPFetchFn } from '..' +import { httpClientOptionsBodyMatcher } from './test-helpers/assert-shape/segment-http-api' +import { createTestAnalytics } from './test-helpers/create-test-analytics' +import { createSuccess } from './test-helpers/factories' + +const testFetch: jest.MockedFn = jest + .fn() + .mockResolvedValue(createSuccess()) + +const assertFetchCallRequest = (url: string, options: RequestInit) => { + expect(url).toBe('https://api.segment.io/v1/batch') + expect(options.headers).toEqual({ + Authorization: 'Basic Zm9vOg==', + 'Content-Type': 'application/json', + 'User-Agent': 'analytics-node-next/latest', + }) + expect(options.method).toBe('POST') + + expect(options.signal).toBeInstanceOf(AbortSignal) +} +describe('HTTP Client', () => { + it('should accept a custom fetch function', async () => { + const analytics = createTestAnalytics({ httpClient: testFetch }) + expect(testFetch).toHaveBeenCalledTimes(0) + await new Promise((resolve) => + analytics.track({ event: 'foo', userId: 'bar' }, resolve) + ) + expect(testFetch).toHaveBeenCalledTimes(1) + assertFetchCallRequest(...testFetch.mock.lastCall) + const [, options] = testFetch.mock.lastCall + const batch = JSON.parse(options.body!).batch + expect(batch.length).toBe(1) + expect(batch[0]).toEqual({ + ...httpClientOptionsBodyMatcher, + timestamp: expect.stringMatching( + /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/ + ), + properties: {}, + type: 'track', + userId: 'bar', + event: 'foo', + }) + }) +}) diff --git a/packages/node/src/__tests__/test-helpers/assert-shape/index.ts b/packages/node/src/__tests__/test-helpers/assert-shape/index.ts index 91df20a72..0ae1c3172 100644 --- a/packages/node/src/__tests__/test-helpers/assert-shape/index.ts +++ b/packages/node/src/__tests__/test-helpers/assert-shape/index.ts @@ -1 +1,2 @@ export * from './http-request-event' +export * from './segment-http-api' diff --git a/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts b/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts similarity index 74% rename from packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts rename to packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts index 9e2ee48f2..17b5b19b2 100644 --- a/packages/node/src/plugins/segmentio/__tests__/test-helpers/segment-http-api.ts +++ b/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts @@ -1,7 +1,10 @@ -import { Context } from '../../../../app/context' -import { HTTPRequestOptions } from '../../../../lib/http-client' +import { Context } from '../../../app/context' +import { HTTPRequestOptions } from '../../../lib/http-client' -export const bodyPropertyMatchers = { +/** + * These map to the data properties of the HTTPClient options (the input value of 'makeRequest') + */ +export const httpClientOptionsBodyMatcher = { messageId: expect.stringMatching(/^node-next-\d*-\w*-\w*-\w*-\w*-\w*/), timestamp: expect.any(Date), _metadata: expect.any(Object), @@ -33,7 +36,7 @@ export function assertHTTPRequestOptions( for (const context of contexts) { expect(data.batch[idx]).toEqual({ ...context.event, - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, }) idx += 1 } diff --git a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts index c188a6752..f1ed369b3 100644 --- a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts @@ -5,9 +5,9 @@ import { PublisherProps } from '../publisher' import { Context } from '../../../app/context' import { Emitter } from '@segment/analytics-core' import { - bodyPropertyMatchers, assertHTTPRequestOptions, -} from './test-helpers/segment-http-api' + httpClientOptionsBodyMatcher, +} from '../../../__tests__/test-helpers/assert-shape' import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-analytics' let emitter: Emitter @@ -57,7 +57,7 @@ test('alias', async () => { expect(data.batch).toHaveLength(1) expect(data.batch[0]).toEqual({ - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, type: 'alias', previousId: 'from', userId: 'to', @@ -87,7 +87,7 @@ test('group', async () => { expect(data.batch).toHaveLength(1) expect(data.batch[0]).toEqual({ - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, traits: { name: 'libraries', }, @@ -115,7 +115,7 @@ test('identify', async () => { const data = request.data expect(data.batch).toHaveLength(1) expect(data.batch[0]).toEqual({ - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, traits: { name: 'Chris Radek', }, @@ -146,7 +146,7 @@ test('page', async () => { expect(data.batch).toHaveLength(1) expect(data.batch[0]).toEqual({ - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, type: 'page', userId: 'foo-user-id', name: 'Home', @@ -180,7 +180,7 @@ test('screen', async () => { expect(data.batch).toHaveLength(1) expect(data.batch[0]).toEqual({ - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, type: 'screen', userId: 'foo-user-id', name: 'Home', @@ -212,7 +212,7 @@ test('track', async () => { expect(data.batch).toHaveLength(1) expect(data.batch[0]).toEqual({ - ...bodyPropertyMatchers, + ...httpClientOptionsBodyMatcher, type: 'track', event: 'test event', userId: 'foo-user-id', diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index 4602c9448..eae47311f 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -10,7 +10,7 @@ import { } from '../../../__tests__/test-helpers/factories' import { TestFetchClient } from '../../../__tests__/test-helpers/create-test-analytics' import { PublisherProps } from '../publisher' -import { assertHTTPRequestOptions } from './test-helpers/segment-http-api' +import { assertHTTPRequestOptions } from '../../../__tests__/test-helpers/assert-shape/segment-http-api' let emitter: Emitter const testClient = new TestFetchClient() From e1da819e04e6ea482ce7a9365187386cfb851719 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 13:58:23 -0500 Subject: [PATCH 14/24] wip --- .../src/__tests__/http-client.integration.test.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/node/src/__tests__/http-client.integration.test.ts b/packages/node/src/__tests__/http-client.integration.test.ts index b6b50ccc7..390df02d4 100644 --- a/packages/node/src/__tests__/http-client.integration.test.ts +++ b/packages/node/src/__tests__/http-client.integration.test.ts @@ -7,7 +7,9 @@ const testFetch: jest.MockedFn = jest .fn() .mockResolvedValue(createSuccess()) -const assertFetchCallRequest = (url: string, options: RequestInit) => { +const assertFetchCallRequest = ( + ...[url, options]: typeof testFetch['mock']['lastCall'] +) => { expect(url).toBe('https://api.segment.io/v1/batch') expect(options.headers).toEqual({ Authorization: 'Basic Zm9vOg==', @@ -18,6 +20,12 @@ const assertFetchCallRequest = (url: string, options: RequestInit) => { expect(options.signal).toBeInstanceOf(AbortSignal) } +const getLastBatch = (): any[] => { + const [, options] = testFetch.mock.lastCall + const batch = JSON.parse(options.body!).batch + return batch +} + describe('HTTP Client', () => { it('should accept a custom fetch function', async () => { const analytics = createTestAnalytics({ httpClient: testFetch }) @@ -27,8 +35,7 @@ describe('HTTP Client', () => { ) expect(testFetch).toHaveBeenCalledTimes(1) assertFetchCallRequest(...testFetch.mock.lastCall) - const [, options] = testFetch.mock.lastCall - const batch = JSON.parse(options.body!).batch + const batch = getLastBatch() expect(batch.length).toBe(1) expect(batch[0]).toEqual({ ...httpClientOptionsBodyMatcher, From e811273517ecba0062b5b0be7d60592adc07de44 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 15:54:26 -0500 Subject: [PATCH 15/24] wip --- .../__tests__/http-client.integration.test.ts | 10 ++++++++-- .../assert-shape/segment-http-api.ts | 4 ++-- packages/node/src/index.ts | 4 ++-- packages/node/src/lib/abort.ts | 2 +- packages/node/src/lib/http-client.ts | 18 +++++++++--------- .../node/src/plugins/segmentio/publisher.ts | 4 ++-- 6 files changed, 24 insertions(+), 18 deletions(-) diff --git a/packages/node/src/__tests__/http-client.integration.test.ts b/packages/node/src/__tests__/http-client.integration.test.ts index 390df02d4..12a0537b1 100644 --- a/packages/node/src/__tests__/http-client.integration.test.ts +++ b/packages/node/src/__tests__/http-client.integration.test.ts @@ -1,8 +1,8 @@ import { HTTPFetchFn } from '..' +import { AbortSignal as AbortSignalShim } from '../lib/abort' import { httpClientOptionsBodyMatcher } from './test-helpers/assert-shape/segment-http-api' import { createTestAnalytics } from './test-helpers/create-test-analytics' import { createSuccess } from './test-helpers/factories' - const testFetch: jest.MockedFn = jest .fn() .mockResolvedValue(createSuccess()) @@ -18,7 +18,13 @@ const assertFetchCallRequest = ( }) expect(options.method).toBe('POST') - expect(options.signal).toBeInstanceOf(AbortSignal) + // @ts-ignore + if (typeof AbortSignal !== 'undefined') { + // @ts-ignore + expect(options.signal).toBeInstanceOf(AbortSignal) + } else { + expect(options.signal).toBeInstanceOf(AbortSignalShim) + } } const getLastBatch = (): any[] => { const [, options] = testFetch.mock.lastCall diff --git a/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts b/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts index 17b5b19b2..5dc2271d7 100644 --- a/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts +++ b/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts @@ -1,5 +1,5 @@ import { Context } from '../../../app/context' -import { HTTPRequestOptions } from '../../../lib/http-client' +import { HTTPClientRequestOptions } from '../../../lib/http-client' /** * These map to the data properties of the HTTPClient options (the input value of 'makeRequest') @@ -18,7 +18,7 @@ export const httpClientOptionsBodyMatcher = { } export function assertHTTPRequestOptions( - { data, headers, method, url }: HTTPRequestOptions, + { data, headers, method, url }: HTTPClientRequestOptions, contexts: Context[] ) { expect(url).toBe('https://api.segment.io/v1/batch') diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 32f71297e..086e165ac 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -4,9 +4,9 @@ export { HTTPClient, FetchHTTPClient, HTTPFetchRequest, - HTTPFetchResponse, + HTTPResponse, HTTPFetchFn, - HTTPRequestOptions, + HTTPClientRequestOptions, } from './lib/http-client' export type { diff --git a/packages/node/src/lib/abort.ts b/packages/node/src/lib/abort.ts index 3a60b3cd4..dae0dd341 100644 --- a/packages/node/src/lib/abort.ts +++ b/packages/node/src/lib/abort.ts @@ -7,7 +7,7 @@ import { detectRuntime } from './env' /** * adapted from: https://www.npmjs.com/package/node-abort-controller */ -class AbortSignal { +export class AbortSignal { onabort: globalThis.AbortSignal['onabort'] = null aborted = false eventEmitter = new Emitter() diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 2fc550850..8620f721e 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -7,11 +7,11 @@ import { fetch as defaultFetch } from './fetch' * @link https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API */ export interface HTTPFetchFn { - (url: string, requestInit: HTTPFetchRequest): Promise + (url: string, requestInit: HTTPFetchRequest): Promise } /** - * This interface is meant to he compatible with the Request interface. + * This interface is meant to be compatible with the Request interface. * @link https://developer.mozilla.org/en-US/docs/Web/API/Request */ export interface HTTPFetchRequest { @@ -22,10 +22,10 @@ export interface HTTPFetchRequest { } /** - * This interface is meant to conform to the Fetch API Response interface. + * This interface is meant to very minimally conform to the Response interface. * @link https://developer.mozilla.org/en-US/docs/Web/API/Response */ -export interface HTTPFetchResponse { +export interface HTTPResponse { ok: boolean status: number statusText: string @@ -35,7 +35,7 @@ export interface HTTPFetchResponse { * This interface is meant to be a generic interface for making HTTP requests. * While it may overlap with fetch's Request interface, it is not coupled to it. */ -export interface HTTPRequestOptions { +export interface HTTPClientRequestOptions { /** * URL to be used for the request * @example 'https://api.segment.io/v1/batch' @@ -63,21 +63,21 @@ export interface HTTPRequestOptions { } /** - * HTTP client interface for making requests.cccccbenidtinulcgklueududltvedrjgeligdefjkfb + * HTTP client interface for making requests */ export interface HTTPClient { - makeRequest(_options: HTTPRequestOptions): Promise + makeRequest(_options: HTTPClientRequestOptions): Promise } /** - * Default HTTP client implementation using fetch. + * Default HTTP client implementation using fetch */ export class FetchHTTPClient implements HTTPClient { private _fetch: HTTPFetchFn constructor(fetchFn?: HTTPFetchFn) { this._fetch = fetchFn ?? defaultFetch } - async makeRequest(options: HTTPRequestOptions): Promise { + async makeRequest(options: HTTPClientRequestOptions): Promise { const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) const requestInit = { diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index a7b466608..1eab1dbbf 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -5,7 +5,7 @@ import { extractPromiseParts } from '../../lib/extract-promise-parts' import { ContextBatch } from './context-batch' import { NodeEmitter } from '../../app/emitter' import { b64encode } from '../../lib/base-64-encode' -import { HTTPClient, HTTPRequestOptions } from '../../lib/http-client' +import { HTTPClient, HTTPClientRequestOptions } from '../../lib/http-client' function sleep(timeoutInMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, timeoutInMs)) @@ -198,7 +198,7 @@ export class Publisher { return batch.resolveEvents() } - const request: HTTPRequestOptions = { + const request: HTTPClientRequestOptions = { timeout: this._httpRequestTimeout, url: this._url, method: 'POST', From 6d628ba0717b7bb6b73b04e7a7f3286ca36c97af Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Fri, 7 Jul 2023 15:57:30 -0500 Subject: [PATCH 16/24] wip --- .../__tests__/test-helpers/assert-shape/segment-http-api.ts | 4 ++-- packages/node/src/index.ts | 2 +- packages/node/src/lib/http-client.ts | 6 +++--- packages/node/src/plugins/segmentio/publisher.ts | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts b/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts index 5dc2271d7..4c6a02743 100644 --- a/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts +++ b/packages/node/src/__tests__/test-helpers/assert-shape/segment-http-api.ts @@ -1,5 +1,5 @@ import { Context } from '../../../app/context' -import { HTTPClientRequestOptions } from '../../../lib/http-client' +import { HTTPClientRequest } from '../../../lib/http-client' /** * These map to the data properties of the HTTPClient options (the input value of 'makeRequest') @@ -18,7 +18,7 @@ export const httpClientOptionsBodyMatcher = { } export function assertHTTPRequestOptions( - { data, headers, method, url }: HTTPClientRequestOptions, + { data, headers, method, url }: HTTPClientRequest, contexts: Context[] ) { expect(url).toBe('https://api.segment.io/v1/batch') diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 086e165ac..e1facda3a 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -6,7 +6,7 @@ export { HTTPFetchRequest, HTTPResponse, HTTPFetchFn, - HTTPClientRequestOptions, + HTTPClientRequest, } from './lib/http-client' export type { diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 8620f721e..27c366170 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -35,7 +35,7 @@ export interface HTTPResponse { * This interface is meant to be a generic interface for making HTTP requests. * While it may overlap with fetch's Request interface, it is not coupled to it. */ -export interface HTTPClientRequestOptions { +export interface HTTPClientRequest { /** * URL to be used for the request * @example 'https://api.segment.io/v1/batch' @@ -66,7 +66,7 @@ export interface HTTPClientRequestOptions { * HTTP client interface for making requests */ export interface HTTPClient { - makeRequest(_options: HTTPClientRequestOptions): Promise + makeRequest(_options: HTTPClientRequest): Promise } /** @@ -77,7 +77,7 @@ export class FetchHTTPClient implements HTTPClient { constructor(fetchFn?: HTTPFetchFn) { this._fetch = fetchFn ?? defaultFetch } - async makeRequest(options: HTTPClientRequestOptions): Promise { + async makeRequest(options: HTTPClientRequest): Promise { const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) const requestInit = { diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index 1eab1dbbf..ab233a210 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -5,7 +5,7 @@ import { extractPromiseParts } from '../../lib/extract-promise-parts' import { ContextBatch } from './context-batch' import { NodeEmitter } from '../../app/emitter' import { b64encode } from '../../lib/base-64-encode' -import { HTTPClient, HTTPClientRequestOptions } from '../../lib/http-client' +import { HTTPClient, HTTPClientRequest } from '../../lib/http-client' function sleep(timeoutInMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, timeoutInMs)) @@ -198,7 +198,7 @@ export class Publisher { return batch.resolveEvents() } - const request: HTTPClientRequestOptions = { + const request: HTTPClientRequest = { timeout: this._httpRequestTimeout, url: this._url, method: 'POST', From e6952cc1ff9f53218f9f94d35471d52b9c5f7b54 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Sun, 9 Jul 2023 13:27:52 -0500 Subject: [PATCH 17/24] wip --- .../__tests__/http-client.integration.test.ts | 80 +++++++++++-------- 1 file changed, 46 insertions(+), 34 deletions(-) diff --git a/packages/node/src/__tests__/http-client.integration.test.ts b/packages/node/src/__tests__/http-client.integration.test.ts index 12a0537b1..d84562fd2 100644 --- a/packages/node/src/__tests__/http-client.integration.test.ts +++ b/packages/node/src/__tests__/http-client.integration.test.ts @@ -1,46 +1,35 @@ -import { HTTPFetchFn } from '..' +import { FetchHTTPClient, HTTPFetchFn } from '..' import { AbortSignal as AbortSignalShim } from '../lib/abort' import { httpClientOptionsBodyMatcher } from './test-helpers/assert-shape/segment-http-api' import { createTestAnalytics } from './test-helpers/create-test-analytics' import { createSuccess } from './test-helpers/factories' + const testFetch: jest.MockedFn = jest .fn() .mockResolvedValue(createSuccess()) -const assertFetchCallRequest = ( - ...[url, options]: typeof testFetch['mock']['lastCall'] -) => { - expect(url).toBe('https://api.segment.io/v1/batch') - expect(options.headers).toEqual({ - Authorization: 'Basic Zm9vOg==', - 'Content-Type': 'application/json', - 'User-Agent': 'analytics-node-next/latest', - }) - expect(options.method).toBe('POST') - - // @ts-ignore - if (typeof AbortSignal !== 'undefined') { - // @ts-ignore - expect(options.signal).toBeInstanceOf(AbortSignal) - } else { - expect(options.signal).toBeInstanceOf(AbortSignalShim) - } -} -const getLastBatch = (): any[] => { - const [, options] = testFetch.mock.lastCall - const batch = JSON.parse(options.body!).batch - return batch -} +let analytics: ReturnType -describe('HTTP Client', () => { - it('should accept a custom fetch function', async () => { - const analytics = createTestAnalytics({ httpClient: testFetch }) - expect(testFetch).toHaveBeenCalledTimes(0) - await new Promise((resolve) => +const helpers = { + makeTrackCall: () => + new Promise((resolve) => analytics.track({ event: 'foo', userId: 'bar' }, resolve) - ) - expect(testFetch).toHaveBeenCalledTimes(1) - assertFetchCallRequest(...testFetch.mock.lastCall) + ), + assertFetchCallRequest: ( + ...[url, options]: typeof testFetch['mock']['lastCall'] + ) => { + expect(url).toBe('https://api.segment.io/v1/batch') + expect(options.headers).toEqual({ + Authorization: 'Basic Zm9vOg==', + 'Content-Type': 'application/json', + 'User-Agent': 'analytics-node-next/latest', + }) + expect(options.method).toBe('POST') + const getLastBatch = (): object[] => { + const [, options] = testFetch.mock.lastCall + const batch = JSON.parse(options.body!).batch + return batch + } const batch = getLastBatch() expect(batch.length).toBe(1) expect(batch[0]).toEqual({ @@ -49,9 +38,32 @@ describe('HTTP Client', () => { /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/ ), properties: {}, + event: 'foo', type: 'track', userId: 'bar', - event: 'foo', }) + // @ts-ignore + expect(options.signal).toBeInstanceOf( + typeof AbortSignal !== 'undefined' ? AbortSignal : AbortSignalShim + ) + }, +} + +describe('httpClient option', () => { + it('can be a regular custom HTTP client', async () => { + analytics = createTestAnalytics({ + httpClient: new FetchHTTPClient(testFetch), + }) + expect(testFetch).toHaveBeenCalledTimes(0) + await helpers.makeTrackCall() + expect(testFetch).toHaveBeenCalledTimes(1) + helpers.assertFetchCallRequest(...testFetch.mock.lastCall) + }) + it('can be a simple function that matches the fetch interface', async () => { + analytics = createTestAnalytics({ httpClient: testFetch }) + expect(testFetch).toHaveBeenCalledTimes(0) + await helpers.makeTrackCall() + expect(testFetch).toHaveBeenCalledTimes(1) + helpers.assertFetchCallRequest(...testFetch.mock.lastCall) }) }) From 7ba6558381cf6888712c7319e2955a8124323609 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 13:50:02 -0500 Subject: [PATCH 18/24] update tests/types and respond to feedback --- packages/node/package.json | 3 +- packages/node/src/__tests__/typedef-tests.ts | 30 +++++++++++++++---- packages/node/src/lib/http-client.ts | 8 ++--- .../node/src/plugins/segmentio/publisher.ts | 2 +- yarn.lock | 16 ++++++++-- 5 files changed, 44 insertions(+), 15 deletions(-) diff --git a/packages/node/package.json b/packages/node/package.json index 47d411b5c..3bebc6d96 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -42,7 +42,8 @@ }, "devDependencies": { "@internal/config": "0.0.0", - "@types/node": "^16" + "@types/node": "^16", + "axios": "^1.4.0" }, "packageManager": "yarn@3.4.1" } diff --git a/packages/node/src/__tests__/typedef-tests.ts b/packages/node/src/__tests__/typedef-tests.ts index 4f3aedf8c..538e0d544 100644 --- a/packages/node/src/__tests__/typedef-tests.ts +++ b/packages/node/src/__tests__/typedef-tests.ts @@ -1,4 +1,5 @@ /* eslint-disable @typescript-eslint/no-var-requires */ +import axios from 'axios' import { Analytics, Context, @@ -7,6 +8,8 @@ import { GroupTraits, HTTPClient, FetchHTTPClient, + HTTPFetchFn, + HTTPClientRequest, } from '../' /** @@ -23,12 +26,6 @@ export default { analytics.VERSION = 'foo' }, - 'httpClient setting should be compatible with standard fetch and node-fetch interface, as well as functions': - () => { - new Analytics({ writeKey: 'foo', httpClient: require('node-fetch') }) - new Analytics({ writeKey: 'foo', httpClient: globalThis.fetch }) - }, - 'Analytics should accept an entire HTTP Client': () => { class CustomClient implements HTTPClient { makeRequest = () => Promise.resolve({} as Response) @@ -92,4 +89,25 @@ export default { console.log({} as GroupTraits) console.log({} as UserTraits) }, + + 'HTTPFetchFn should be compatible with standard fetch and node-fetch interface, as well as functions': + () => { + const fetch: HTTPFetchFn = require('node-fetch') + new Analytics({ writeKey: 'foo', httpClient: fetch }) + new Analytics({ writeKey: 'foo', httpClient: globalThis.fetch }) + }, + + 'httpClient setting should be compatible with axios': () => { + new (class implements HTTPClient { + async makeRequest(options: HTTPClientRequest) { + return axios({ + url: options.url, + method: options.method, + data: options.data, + headers: options.headers, + timeout: options.timeout, + }) + } + })() + }, } diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 27c366170..dee82781f 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -17,7 +17,7 @@ export interface HTTPFetchFn { export interface HTTPFetchRequest { headers?: Record body?: string - method?: string + method?: HTTPClientRequest['method'] signal?: any // AbortSignal type does not play nicely with node-fetch } @@ -26,7 +26,6 @@ export interface HTTPFetchRequest { * @link https://developer.mozilla.org/en-US/docs/Web/API/Response */ export interface HTTPResponse { - ok: boolean status: number statusText: string } @@ -42,10 +41,9 @@ export interface HTTPClientRequest { */ url: string /** - * HTTP method to be used for the request - * @example 'POST' + * HTTP method to be used for the request. This will always be a 'POST' request. **/ - method: string + method: 'POST' /** * Headers to be sent with the request */ diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index ab233a210..be3691767 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -219,7 +219,7 @@ export class Publisher { const response = await this._httpClient.makeRequest(request) - if (response.ok) { + if (response.status >= 200 && response.status < 300) { // Successfully sent events, so exit! batch.resolveEvents() return diff --git a/yarn.lock b/yarn.lock index f137b6e7e..4109af61b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1790,6 +1790,7 @@ __metadata: "@lukeed/uuid": ^2.0.0 "@segment/analytics-core": 1.3.0 "@types/node": ^16 + axios: ^1.4.0 buffer: ^6.0.3 node-fetch: ^2.6.7 tslib: ^2.4.1 @@ -4609,6 +4610,17 @@ __metadata: languageName: node linkType: hard +"axios@npm:^1.4.0": + version: 1.4.0 + resolution: "axios@npm:1.4.0" + dependencies: + follow-redirects: ^1.15.0 + form-data: ^4.0.0 + proxy-from-env: ^1.1.0 + checksum: 7fb6a4313bae7f45e89d62c70a800913c303df653f19eafec88e56cea2e3821066b8409bc68be1930ecca80e861c52aa787659df0ffec6ad4d451c7816b9386b + languageName: node + linkType: hard + "axobject-query@npm:^2.2.0": version: 2.2.0 resolution: "axobject-query@npm:2.2.0" @@ -7394,7 +7406,7 @@ __metadata: languageName: node linkType: hard -"follow-redirects@npm:^1.0.0, follow-redirects@npm:^1.14.9": +"follow-redirects@npm:^1.0.0, follow-redirects@npm:^1.14.9, follow-redirects@npm:^1.15.0": version: 1.15.2 resolution: "follow-redirects@npm:1.15.2" peerDependenciesMeta: @@ -11432,7 +11444,7 @@ __metadata: languageName: node linkType: hard -"proxy-from-env@npm:1.1.0": +"proxy-from-env@npm:1.1.0, proxy-from-env@npm:^1.1.0": version: 1.1.0 resolution: "proxy-from-env@npm:1.1.0" checksum: ed7fcc2ba0a33404958e34d95d18638249a68c430e30fcb6c478497d72739ba64ce9810a24f53a7d921d0c065e5b78e3822759800698167256b04659366ca4d4 From bc4786e626dc2c31f1b57405dd14e3a0e294d519 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:01:29 -0500 Subject: [PATCH 19/24] add test --- .../segmentio/__tests__/publisher.test.ts | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index eae47311f..c95462976 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -308,13 +308,15 @@ describe('error handling', () => { `) }) - it('retries non-400 errors', async () => { + it.each([ + { status: 500, statusText: 'Internal Server Error' }, + { status: 300, statusText: 'Multiple Choices' }, + { status: 100, statusText: 'Continue' }, + ])('retries non-400 errors: %p', async (response) => { // Jest kept timing out when using fake timers despite advancing time. jest.useRealTimers() - fetcher.mockReturnValue( - createError({ status: 500, statusText: 'Internal Server Error' }) - ) + fetcher.mockReturnValue(createError(response)) const { plugin: segmentPlugin } = createTestNodePlugin({ maxRetries: 2, @@ -331,13 +333,12 @@ describe('error handling', () => { expect(updatedContext).toBe(context) expect(updatedContext.failedDelivery()).toBeTruthy() - expect(updatedContext.failedDelivery()).toMatchInlineSnapshot(` - Object { - "reason": [Error: [500] Internal Server Error], - } - `) + const err = updatedContext.failedDelivery()?.reason as Error + expect(err).toBeInstanceOf(Error) + expect(err.message).toEqual( + expect.stringContaining(response.status.toString()) + ) }) - it('retries fetch errors', async () => { // Jest kept timing out when using fake timers despite advancing time. jest.useRealTimers() From 5fdfbad176c7ec7e0ce901355da8497e01339918 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:14:13 -0500 Subject: [PATCH 20/24] change timeout to 'httpRequestTimeout' --- packages/node/src/__tests__/typedef-tests.ts | 2 +- packages/node/src/lib/http-client.ts | 8 +++++--- packages/node/src/plugins/segmentio/publisher.ts | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/node/src/__tests__/typedef-tests.ts b/packages/node/src/__tests__/typedef-tests.ts index 538e0d544..c21f363e3 100644 --- a/packages/node/src/__tests__/typedef-tests.ts +++ b/packages/node/src/__tests__/typedef-tests.ts @@ -105,7 +105,7 @@ export default { method: options.method, data: options.data, headers: options.headers, - timeout: options.timeout, + timeout: options.httpRequestTimeout, }) } })() diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index dee82781f..849492f77 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -54,10 +54,10 @@ export interface HTTPClientRequest { */ data: Record /** - * Timeout in milliseconds + * Specifies the timeout for a HTTP client to get an HTTP response from HTTP server * @example 10000 */ - timeout: number + httpRequestTimeout: number } /** @@ -76,7 +76,9 @@ export class FetchHTTPClient implements HTTPClient { this._fetch = fetchFn ?? defaultFetch } async makeRequest(options: HTTPClientRequest): Promise { - const [signal, timeoutId] = abortSignalAfterTimeout(options.timeout) + const [signal, timeoutId] = abortSignalAfterTimeout( + options.httpRequestTimeout + ) const requestInit = { url: options.url, diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index be3691767..8ad57934d 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -199,7 +199,6 @@ export class Publisher { } const request: HTTPClientRequest = { - timeout: this._httpRequestTimeout, url: this._url, method: 'POST', headers: { @@ -208,6 +207,7 @@ export class Publisher { 'User-Agent': 'analytics-node-next/latest', }, data: { batch: events }, + httpRequestTimeout: this._httpRequestTimeout, } this._emitter.emit('http_request', { From 59411801121920fe4eb71b64681c21f7c9bf6ac8 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:17:15 -0500 Subject: [PATCH 21/24] update docstring --- packages/node/src/lib/http-client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 849492f77..2494896b6 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -54,7 +54,7 @@ export interface HTTPClientRequest { */ data: Record /** - * Specifies the timeout for a HTTP client to get an HTTP response from HTTP server + * Specifies the timeout (in milliseconds) for an HTTP client to get an HTTP response from HTTP server * @example 10000 */ httpRequestTimeout: number From 977d1896fefa38b15b045fcf7b165785903a0f14 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:19:02 -0500 Subject: [PATCH 22/24] wip --- packages/node/src/app/settings.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/node/src/app/settings.ts b/packages/node/src/app/settings.ts index e66bc38ac..0f2ee02f6 100644 --- a/packages/node/src/app/settings.ts +++ b/packages/node/src/app/settings.ts @@ -6,7 +6,6 @@ export interface AnalyticsSettings { * Key that corresponds to your Segment.io project */ writeKey: string - /** /** * The base URL of the API. Default: "https://api.segment.io" */ From d3d694fd655275fc3bb977a9376a9749767e1673 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:19:21 -0500 Subject: [PATCH 23/24] wip --- packages/node/src/lib/http-client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 2494896b6..87cd02465 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -54,7 +54,7 @@ export interface HTTPClientRequest { */ data: Record /** - * Specifies the timeout (in milliseconds) for an HTTP client to get an HTTP response from HTTP server + * Specifies the timeout (in milliseconds) for an HTTP client to get an HTTP response from the server * @example 10000 */ httpRequestTimeout: number From 84ac43b895aea16d8d75d122f522275a978796b6 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Wed, 12 Jul 2023 14:51:55 -0500 Subject: [PATCH 24/24] wip --- packages/node/src/__tests__/typedef-tests.ts | 7 +++++++ packages/node/src/lib/__tests__/abort.test.ts | 4 +++- packages/node/src/lib/http-client.ts | 8 ++++---- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/node/src/__tests__/typedef-tests.ts b/packages/node/src/__tests__/typedef-tests.ts index c21f363e3..2f8e626f6 100644 --- a/packages/node/src/__tests__/typedef-tests.ts +++ b/packages/node/src/__tests__/typedef-tests.ts @@ -97,6 +97,13 @@ export default { new Analytics({ writeKey: 'foo', httpClient: globalThis.fetch }) }, + 'HTTPFetchFn options should be the expected type': () => { + type BadFetch = (url: string, requestInit: { _bad_object?: string }) => any + + // @ts-expect-error + new Analytics({ writeKey: 'foo', httpClient: {} as BadFetch }) + }, + 'httpClient setting should be compatible with axios': () => { new (class implements HTTPClient { async makeRequest(options: HTTPClientRequest) { diff --git a/packages/node/src/lib/__tests__/abort.test.ts b/packages/node/src/lib/__tests__/abort.test.ts index ecc4af764..54f350252 100644 --- a/packages/node/src/lib/__tests__/abort.test.ts +++ b/packages/node/src/lib/__tests__/abort.test.ts @@ -1,7 +1,9 @@ import { abortSignalAfterTimeout } from '../abort' import nock from 'nock' -import { fetch } from '../fetch' import { sleep } from '@segment/analytics-core' +import { fetch as _fetch } from '../fetch' + +const fetch = _fetch as typeof globalThis.fetch describe(abortSignalAfterTimeout, () => { const HOST = 'https://foo.com' diff --git a/packages/node/src/lib/http-client.ts b/packages/node/src/lib/http-client.ts index 87cd02465..93c1c5a96 100644 --- a/packages/node/src/lib/http-client.ts +++ b/packages/node/src/lib/http-client.ts @@ -15,10 +15,10 @@ export interface HTTPFetchFn { * @link https://developer.mozilla.org/en-US/docs/Web/API/Request */ export interface HTTPFetchRequest { - headers?: Record - body?: string - method?: HTTPClientRequest['method'] - signal?: any // AbortSignal type does not play nicely with node-fetch + headers: Record + body: string + method: HTTPClientRequest['method'] + signal: any // AbortSignal type does not play nicely with node-fetch } /**