From db7d425de54f7b73fdca2cb24db19c42fb129fa9 Mon Sep 17 00:00:00 2001 From: Michael Grosse Huelsewiesche Date: Fri, 30 Jun 2023 20:11:48 -0400 Subject: [PATCH] Addressing some PR comments, mostly renaming things --- packages/node/src/__tests__/callback.test.ts | 4 +-- .../src/__tests__/disable.integration.test.ts | 6 ++-- .../src/__tests__/emitter.integration.test.ts | 4 +-- .../graceful-shutdown-integration.test.ts | 8 ++--- .../node/src/__tests__/integration.test.ts | 6 ++-- .../src/__tests__/test-helpers/factories.ts | 2 +- packages/node/src/app/analytics-node.ts | 4 +-- packages/node/src/app/settings.ts | 6 ++-- packages/node/src/lib/__tests__/abort.test.ts | 6 ++-- packages/node/src/lib/custom-http-client.ts | 20 +++++++++++++ .../segmentio/__tests__/methods.test.ts | 12 ++++---- .../segmentio/__tests__/publisher.test.ts | 30 +++++++++---------- .../node/src/plugins/segmentio/publisher.ts | 12 ++++---- 13 files changed, 70 insertions(+), 50 deletions(-) create mode 100644 packages/node/src/lib/custom-http-client.ts diff --git a/packages/node/src/__tests__/callback.test.ts b/packages/node/src/__tests__/callback.test.ts index 52a74f8fd..cf19c090b 100644 --- a/packages/node/src/__tests__/callback.test.ts +++ b/packages/node/src/__tests__/callback.test.ts @@ -15,7 +15,7 @@ describe('Callback behavior', () => { it('should handle success', async () => { const ajs = createTestAnalytics({ maxEventsInBatch: 1, - customClient: testClient, + httpClient: testClient, }) const ctx = await new Promise((resolve, reject) => ajs.track( @@ -37,7 +37,7 @@ describe('Callback behavior', () => { testClient.returnValue = createError() const ajs = createTestAnalytics({ maxEventsInBatch: 1, - customClient: testClient, + httpClient: testClient, }) 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 e6c68688a..e28abc64b 100644 --- a/packages/node/src/__tests__/disable.integration.test.ts +++ b/packages/node/src/__tests__/disable.integration.test.ts @@ -1,5 +1,5 @@ import { createTestAnalytics } from './test-helpers/create-test-analytics' -import { CustomHTTPClient } from '../lib/customhttpclient' +import { CustomHTTPClient } from '../lib/custom-http-client' export class CheckFetchClient implements CustomHTTPClient { private _wasCalled = false @@ -38,7 +38,7 @@ describe('disable', () => { it('should call fetch if disabled is false', async () => { const analytics = createTestAnalytics({ disable: false, - customClient: checkFetchClient, + httpClient: checkFetchClient, }) checkFetchClient.wasCalled = false await new Promise((resolve) => @@ -49,7 +49,7 @@ describe('disable', () => { it('should not call fetch if disabled is true', async () => { const analytics = createTestAnalytics({ disable: true, - customClient: checkFetchClient, + httpClient: checkFetchClient, }) checkFetchClient.wasCalled = false await new Promise((resolve) => diff --git a/packages/node/src/__tests__/emitter.integration.test.ts b/packages/node/src/__tests__/emitter.integration.test.ts index 25ed8cbfa..b1ddc4427 100644 --- a/packages/node/src/__tests__/emitter.integration.test.ts +++ b/packages/node/src/__tests__/emitter.integration.test.ts @@ -24,7 +24,7 @@ describe('http_request', () => { testClient.returnValue = createError() const analytics = createTestAnalytics({ maxRetries: 0, - customClient: testClient, + httpClient: testClient, }) const fn = jest.fn() analytics.on('http_request', fn) @@ -38,7 +38,7 @@ describe('http_request', () => { testClient.returnValue = createError() const analytics = createTestAnalytics({ maxRetries: 2, - customClient: testClient, + httpClient: testClient, }) const fn = jest.fn() analytics.on('http_request', fn) diff --git a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts index f07ad3bc1..33da394a2 100644 --- a/packages/node/src/__tests__/graceful-shutdown-integration.test.ts +++ b/packages/node/src/__tests__/graceful-shutdown-integration.test.ts @@ -22,7 +22,7 @@ describe('Ability for users to exit without losing events', () => { ajs = new Analytics({ writeKey: 'abc123', maxEventsInBatch: 1, - customClient: testClient, + httpClient: testClient, }) }) const _helpers = { @@ -88,7 +88,7 @@ describe('Ability for users to exit without losing events', () => { ajs = new Analytics({ writeKey: 'abc123', flushInterval, - customClient: testClient, + httpClient: testClient, }) const closeAndFlushTimeout = ajs['_closeAndFlushDefaultTimeout'] expect(closeAndFlushTimeout).toBe(flushInterval * 1.25) @@ -190,7 +190,7 @@ describe('Ability for users to exit without losing events', () => { writeKey: 'foo', flushInterval: 10000, maxEventsInBatch: 15, - customClient: testClient, + httpClient: testClient, }) _helpers.makeTrackCall(analytics) _helpers.makeTrackCall(analytics) @@ -221,7 +221,7 @@ describe('Ability for users to exit without losing events', () => { writeKey: 'foo', flushInterval: 10000, maxEventsInBatch: 15, - customClient: testClient, + httpClient: testClient, }) await analytics.register(_testPlugin) _helpers.makeTrackCall(analytics) diff --git a/packages/node/src/__tests__/integration.test.ts b/packages/node/src/__tests__/integration.test.ts index 5278e8a11..5dfee106c 100644 --- a/packages/node/src/__tests__/integration.test.ts +++ b/packages/node/src/__tests__/integration.test.ts @@ -27,7 +27,7 @@ describe('Settings / Configuration Init', () => { const analytics = createTestAnalytics({ host: 'http://foo.com', path: '/bar', - customClient: testClient, + httpClient: testClient, }) const track = resolveCtx(analytics, 'track') analytics.track({ event: 'foo', userId: 'sup' }) @@ -52,10 +52,10 @@ describe('Error handling', () => { expect(() => analytics.track({} as any)).toThrowError(/event/i) }) - it.only('should emit on an error', async () => { + it('should emit on an error', async () => { const analytics = createTestAnalytics({ maxRetries: 0, - customClient: testClient, + httpClient: testClient, }) testClient.returnValue = createError({ statusText: 'Service Unavailable', diff --git a/packages/node/src/__tests__/test-helpers/factories.ts b/packages/node/src/__tests__/test-helpers/factories.ts index 72249f8a3..c03f74b98 100644 --- a/packages/node/src/__tests__/test-helpers/factories.ts +++ b/packages/node/src/__tests__/test-helpers/factories.ts @@ -1,4 +1,4 @@ -import { CustomHTTPClient } from '../../lib/customhttpclient' +import { CustomHTTPClient } from '../../lib/custom-http-client' export const createSuccess = (body?: any) => { return Promise.resolve({ diff --git a/packages/node/src/app/analytics-node.ts b/packages/node/src/app/analytics-node.ts index 39b35598d..44a89f41c 100644 --- a/packages/node/src/app/analytics-node.ts +++ b/packages/node/src/app/analytics-node.ts @@ -16,7 +16,7 @@ import { } from './types' import { Context } from './context' import { NodeEventQueue } from './event-queue' -import { DefaultFetchClient } from '../lib/customhttpclient' +import { DefaultHttpClient } from '../lib/custom-http-client' export class Analytics extends NodeEmitter implements CoreAnalytics { private readonly _eventFactory: NodeEventFactory @@ -52,7 +52,7 @@ export class Analytics extends NodeEmitter implements CoreAnalytics { httpRequestTimeout: settings.httpRequestTimeout, disable: settings.disable, flushInterval, - customclient: settings.customClient ?? new DefaultFetchClient(), + httpClient: settings.httpClient ?? new DefaultHttpClient(), }, this as NodeEmitter ) diff --git a/packages/node/src/app/settings.ts b/packages/node/src/app/settings.ts index 55dd3e67a..6e721cfbd 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 { CustomHTTPClient } from '../lib/customhttpclient' +import { CustomHTTPClient } from '../lib/custom-http-client' export interface AnalyticsSettings { /** @@ -38,10 +38,10 @@ export interface AnalyticsSettings { /** * Supply a CustomHTTPClient implementation (such as one supporting proxy) - * Default: DefaultFetchClient which will use the existing global fetch, or + * Default: DefaultHttpClient which will use the existing global fetch, or * node-fetch if it doesn't exist */ - customClient?: CustomHTTPClient + httpClient?: CustomHTTPClient } export const validateSettings = (settings: AnalyticsSettings) => { diff --git a/packages/node/src/lib/__tests__/abort.test.ts b/packages/node/src/lib/__tests__/abort.test.ts index ec154ed6b..ec4ac8967 100644 --- a/packages/node/src/lib/__tests__/abort.test.ts +++ b/packages/node/src/lib/__tests__/abort.test.ts @@ -1,7 +1,7 @@ import { abortSignalAfterTimeout } from '../abort' import nock from 'nock' import { sleep } from '@segment/analytics-core' -import { DefaultFetchClient } from '../customhttpclient' +import { DefaultHttpClient } from '../custom-http-client' describe(abortSignalAfterTimeout, () => { const HOST = 'https://foo.com' @@ -30,7 +30,7 @@ describe(abortSignalAfterTimeout, () => { try { const [signal] = abortSignalAfterTimeout(2000) jest.advanceTimersByTime(6000) - const client = new DefaultFetchClient() + const client = new DefaultHttpClient() await client.send(HOST, { signal }) throw Error('fail test.') } catch (err: any) { @@ -43,7 +43,7 @@ describe(abortSignalAfterTimeout, () => { nock(HOST).get('/').reply(201) const [signal] = abortSignalAfterTimeout(0) try { - const client = new DefaultFetchClient() + const client = new DefaultHttpClient() await client.send(HOST, { signal }) throw Error('fail test.') } catch (err: any) { diff --git a/packages/node/src/lib/custom-http-client.ts b/packages/node/src/lib/custom-http-client.ts new file mode 100644 index 000000000..7c3a4104c --- /dev/null +++ b/packages/node/src/lib/custom-http-client.ts @@ -0,0 +1,20 @@ +export interface CustomHTTPClient { + send: (resource: any, options: any) => Promise +} + +export class DefaultHttpClient implements CustomHTTPClient { + send = async (resource: any, options: any): Promise => { + if (globalThis.fetch) { + return globalThis.fetch(resource, options) + } // @ts-ignore + // This guard causes is important, as it causes dead-code elimination to be enabled inside this block. + else if (typeof EdgeRuntime !== 'string') { + // @ts-ignore + return (await import('node-fetch')).default(payload, options) as Response + } else { + throw new Error( + 'Invariant: an edge runtime that does not support fetch should not exist' + ) + } + } +} diff --git a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts index 5b039254c..a3a90230c 100644 --- a/packages/node/src/plugins/segmentio/__tests__/methods.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/methods.test.ts @@ -34,7 +34,7 @@ test('alias', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const event = eventFactory.alias('to', 'from') @@ -65,7 +65,7 @@ test('group', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const event = eventFactory.group( @@ -105,7 +105,7 @@ test('identify', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const event = eventFactory.identify('foo-user-id', { @@ -138,7 +138,7 @@ test('page', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const event = eventFactory.page( @@ -178,7 +178,7 @@ test('screen', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const event = eventFactory.screen( @@ -217,7 +217,7 @@ test('track', async () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const event = eventFactory.track( diff --git a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts index b2009f71c..d4a3e3a3b 100644 --- a/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts +++ b/packages/node/src/plugins/segmentio/__tests__/publisher.test.ts @@ -36,7 +36,7 @@ it('supports multiple events in a batch', async () => { maxEventsInBatch: 3, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) // Create 3 events of mixed types to send. @@ -65,7 +65,7 @@ it('supports waiting a max amount of time before sending', async () => { maxEventsInBatch: 3, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const context = new Context(eventFactory.alias('to', 'from')) @@ -94,7 +94,7 @@ it('sends as soon as batch fills up or max time is reached', async () => { maxEventsInBatch: 2, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const context = new Context(eventFactory.alias('to', 'from')) @@ -131,7 +131,7 @@ it('sends if batch will exceed max size in bytes when adding event', async () => maxEventsInBatch: 20, flushInterval: 100, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const contexts: Context[] = [] @@ -188,7 +188,7 @@ describe('flushAfterClose', () => { maxEventsInBatch: 20, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) publisher.flushAfterClose(3) @@ -206,7 +206,7 @@ describe('flushAfterClose', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) publisher.flushAfterClose(3) @@ -223,7 +223,7 @@ describe('flushAfterClose', () => { maxEventsInBatch: 3, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) publisher.flushAfterClose(5) @@ -239,7 +239,7 @@ describe('flushAfterClose', () => { maxEventsInBatch: 7, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) range(3).forEach(() => segmentPlugin.track(_createTrackCtx())) // should not flush @@ -256,7 +256,7 @@ describe('flushAfterClose', () => { maxEventsInBatch: 7, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) range(3).forEach(() => segmentPlugin.track(_createTrackCtx())) // should not flush @@ -279,7 +279,7 @@ describe('error handling', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const context = new Context( @@ -317,7 +317,7 @@ describe('error handling', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const context = new Context(eventFactory.alias('to', 'from')) @@ -350,7 +350,7 @@ describe('error handling', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const context = new Context(eventFactory.alias('to', 'from')) @@ -381,7 +381,7 @@ describe('error handling', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const context = new Context(eventFactory.alias('my', 'from')) @@ -411,7 +411,7 @@ describe('error handling', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const fn = jest.fn() @@ -437,7 +437,7 @@ describe('http_request emitter event', () => { maxEventsInBatch: 1, flushInterval: 1000, writeKey: '', - customclient: testClient, + httpClient: testClient, }) const fn = jest.fn() diff --git a/packages/node/src/plugins/segmentio/publisher.ts b/packages/node/src/plugins/segmentio/publisher.ts index 7f6fc8a05..c7f4bf602 100644 --- a/packages/node/src/plugins/segmentio/publisher.ts +++ b/packages/node/src/plugins/segmentio/publisher.ts @@ -6,7 +6,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 { CustomHTTPClient } from '../../lib/customhttpclient' +import { CustomHTTPClient } from '../../lib/custom-http-client' function sleep(timeoutInMs: number): Promise { return new Promise((resolve) => setTimeout(resolve, timeoutInMs)) @@ -28,7 +28,7 @@ export interface PublisherProps { writeKey: string httpRequestTimeout?: number disable?: boolean - customclient: CustomHTTPClient + httpClient: CustomHTTPClient } /** @@ -47,7 +47,7 @@ export class Publisher { private _httpRequestTimeout: number private _emitter: NodeEmitter private _disable: boolean - public customclient: CustomHTTPClient + public httpClient: CustomHTTPClient constructor( { host, @@ -57,7 +57,7 @@ export class Publisher { flushInterval, writeKey, httpRequestTimeout, - customclient: client, + httpClient: client, disable, }: PublisherProps, emitter: NodeEmitter @@ -73,7 +73,7 @@ export class Publisher { ) this._httpRequestTimeout = httpRequestTimeout ?? 10000 this._disable = Boolean(disable) - this.customclient = client + this.httpClient = client } private createBatch(): ContextBatch { @@ -222,7 +222,7 @@ export class Publisher { return batch.resolveEvents() } - const response = await this.customclient.send(this._url, requestInit) + const response = await this.httpClient.send(this._url, requestInit) clearTimeout(timeoutId)