Update cb behavior (#692)

Author: silesky

.changeset/cyan-rocks-happen.md

@@ -0,0 +1,5 @@
+---
+'@segment/analytics-core': patch
+---
+
+Move code out of core and into analytics-node. Tweak emitter error contract.

packages/core/src/analytics/dispatch-emit.ts

@@ -1,29 +1,23 @@
 import { CoreContext } from '../context'
 
-export type EmittedUnknownError<Ctx extends CoreContext> = {
-  code: 'unknown'
-  message: string
-  ctx?: Ctx
-  err?: any
-}
-
-export type EmittedDeliveryFailureError<Ctx extends CoreContext> = {
-  code: 'delivery_failure'
-  message: string
-  ctx: Ctx
-  data?: any
-}
-
 /**
- * Discriminated of all errors with a discriminant key of "code"
+ * This is the base contract for all emitted errors. This interface may be extended.
  */
-export type CoreEmittedError<Ctx extends CoreContext> =
-  | EmittedUnknownError<Ctx>
-  | EmittedDeliveryFailureError<Ctx>
+export interface CoreEmittedError<Ctx extends CoreContext> {
+  /**
+   * e.g. 'delivery_failure'
+   */
+  code: string
+  /**
+   * Why the error occurred. This can be an actual error object or a just a message.
+   */
+  reason?: unknown
+  ctx?: Ctx
+}
 
 export type CoreEmitterContract<
   Ctx extends CoreContext,
-  AdditionalErrors = CoreEmittedError<Ctx>
+  Err extends CoreEmittedError<Ctx> = CoreEmittedError<Ctx>
 > = {
   alias: [ctx: Ctx]
   track: [ctx: Ctx]
@@ -33,5 +27,5 @@ export type CoreEmitterContract<
   group: [ctx: Ctx]
   register: [pluginNames: string[]]
   deregister: [pluginNames: string[]]
-  error: [CoreEmittedError<Ctx> | AdditionalErrors]
+  error: [error: Err]
 }

packages/core/src/emitter/interface.ts

@@ -10,7 +10,6 @@ export * from './context'
 export * from './queue/event-queue'
 export * from './analytics'
 export * from './analytics/dispatch'
-export * from './analytics/dispatch-emit'
 export * from './validation/helpers'
 export * from './validation/assertions'
 export * from './utils/bind-all'

packages/core/src/index.ts

@@ -36,7 +36,7 @@ app.post('/cart', (req, res) => {
     event: 'Add to cart',
     properties: { productId: '123456' }
   })
-   res.sendStatus(200)
+   res.sendStatus(201)
 });
 ```
 ## Regional configuration
@@ -48,8 +48,9 @@ Dublin — events.eu1.segmentapis.com
 An example of setting the host to the EU endpoint using the Node library would be:
 
 ```ts
-const analytics = new Analytics('YOUR_WRITE_KEY', {
-    host: "https://events.eu1.segmentapis.com"
+const analytics = new Analytics({
+  ...
+  host: "https://events.eu1.segmentapis.com"
 });
 ```
 
@@ -66,7 +67,6 @@ const analytics = new Analytics({
     flushInterval: 10000,
     // ... and more!
   })
-
 ```
 
 ## Batching
@@ -84,16 +84,22 @@ There is a maximum of 500KB per batch request and 32KB per call.
 
 If you don’t want to batch messages, you can turn batching off by setting the `maxEventsInBatch` setting to 1, like so:
 ```ts
-const analytics = new Analytics({ '<MY_WRITE_KEY>', { maxEventsInBatch: 1 });
+const analytics = new Analytics({
+  ...
+  maxEventsInBatch: 1
+});
 ```
 Batching means that your message might not get sent right away. But every method call takes an optional callback, which you can use to know when a particular message is flushed from the queue, like so:
 
 ```ts
 analytics.track({
-  userId: '019mr8mf4r',
-  event: 'Ultimate Played'
-  callback: (ctx) => console.log(ctx)
-})
+    userId: '019mr8mf4r',
+    event: 'Ultimate Played',
+  },
+  (err, ctx) => {
+    ...
+  }
+)
 ```
 ## Error Handling
 Subscribe and log all event delivery errors.
@@ -143,7 +149,6 @@ const onExit = async () => {
 }
 
 ['SIGINT', 'SIGTERM'].forEach((code) => process.on(code, onExit))
-
 ```
 
 #### Collecting unflushed events
@@ -175,16 +180,16 @@ Different parts of your application may require different types of batching, or
 ```ts
 import { Analytics } from '@segment/analytics-node'
 
-const marketingAnalytics = new Analytics('MARKETING_WRITE_KEY');
-const appAnalytics = new Analytics('APP_WRITE_KEY');
+const marketingAnalytics = new Analytics({ writeKey: 'MARKETING_WRITE_KEY' });
+const appAnalytics = new Analytics({ writeKey: 'APP_WRITE_KEY' });
 ```
 
 ## Troubleshooting
 1. Double check that you’ve followed all the steps in the Quick Start.
 
 2. Make sure that you’re calling a Segment API method once the library is successfully installed: identify, track, etc.
 
-3. Log events and errors the event emitter:
+3. Log events.
 ```js
 ['initialize', 'call_after_close',
  'screen', 'identify', 'group',
@@ -194,6 +199,20 @@ const appAnalytics = new Analytics('APP_WRITE_KEY');
 ```
 
 
+## Development: Disabling Analytics for Tests
+- If you want to intercept / disable analytics for integration tests, you can use something like [nock](https://github.com/nock/nock)
+
+```ts
+// Note: nock will _not_ work if polyfill fetch with something like undici, as nock uses the http module. Undici has its own interception method.
+import nock from 'nock'
+
+nock('https://api.segment.io')
+  .post('/v1/batch')
+  .reply(201)
+  .persist()
+```
+
+
 ## Differences from legacy analytics-node / Migration Guide
 
 
@@ -206,15 +225,13 @@ import Analytics from 'analytics-node'
 import { Analytics } from '@segment/analytics-next'
 ```
 
-- Instantiation requires an object
+- Instantiation now requires an _object_ as the first argument.
 ```ts
 // old
+var analytics = new Analytics('YOUR_WRITE_KEY'); // not supported
 
-var analytics = new Analytics('YOUR_WRITE_KEY');
-
-// new
-const analytics = new Analytics({ writeKey: 'YOUR_WRITE_KEY' });
-
+// new!
+const analytics = new Analytics({ writeKey: '<MY_WRITE_KEY>' })
 ```
 - Graceful shutdown (See Graceful Shutdown section)
 ```ts
@@ -232,49 +249,10 @@ Other Differences:
 - The `enable` configuration option has been removed-- see "Disabling Analytics" section
 - the `errorHandler` configuration option has been remove  -- see "Error Handling" section
 - `flushAt` configuration option -> `maxEventsInBatch`.
-- `callback` option is moved to configuration
+- `callback` call signature is different
 ```ts
 // old
-analytics.track({
-  userId: '019mr8mf4r',
-  event: 'Ultimate Played'
-}), function(err, batch){
-  if (err) {
-    console.error(err)
-  }
-});
-
+(err, batch) => void
 // new
-analytics.track({
-  userId: '019mr8mf4r',
-  event: 'Ultimate Played',
-  callback: (ctx) => {
-     if (ctx.failedDelivery()) {
-        console.error(ctx)
-     }
-  }
-})
-
-```
-
-
-## Development / Disabling Analytics
-- If you want to disable analytics for unit tests, you can use something like [nock](https://github.com/nock/nock) or [jest mocks](https://jestjs.io/docs/manual-mocks).
-
-You should prefer mocking. However, if you need to intercept the request, you can do:
-
-```ts
-  // Note: nock will _not_ work if polyfill fetch with something like undici, as nock uses the http module. Undici has its own interception method.
-  import nock from 'nock'
-
-  const mockApiHost = 'https://foo.bar'
-  const mockPath = '/foo'
-
-  nock(mockApiHost) // using regex matching in nock changes the perf profile quite a bit
-    .post(mockPath, (body) => true)
-    .reply(201)
-    .persist()
-
-const analytics = new Analytics({ host: mockApiHost, path: mockPath })
-
+(err, ctx) => void
 ```

packages/node/README.md

@@ -0,0 +1,49 @@
+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/analytics-node'
+
+describe('Callback behavior', () => {
+  beforeEach(() => {
+    fetcher.mockReturnValue(createSuccess())
+  })
+
+  it('should handle success', async () => {
+    const ajs = createTestAnalytics({ maxEventsInBatch: 1 })
+    const ctx = await new Promise<Context>((resolve, reject) =>
+      ajs.track(
+        {
+          anonymousId: 'bar',
+          event: 'event name',
+        },
+        (err, ctx) => {
+          if (err) reject('test fail')
+          resolve(ctx!)
+        }
+      )
+    )
+    expect(ctx.event.event).toBe('event name')
+    expect(ctx.event.anonymousId).toBe('bar')
+  })
+
+  it('should handle errors', async () => {
+    fetcher.mockReturnValue(createError())
+    const ajs = createTestAnalytics({ maxEventsInBatch: 1 })
+    const [err, ctx] = await new Promise<[any, Context]>((resolve) =>
+      ajs.track(
+        {
+          anonymousId: 'bar',
+          event: 'event name',
+        },
+        (err, ctx) => {
+          resolve([err!, ctx!])
+        }
+      )
+    )
+    expect(ctx.event.event).toBe('event name')
+    expect(ctx.event.anonymousId).toBe('bar')
+    expect(err).toEqual(new Error('[404] Not Found'))
+  })
+})

packages/node/src/__tests__/callback.test.ts

@@ -26,7 +26,7 @@ describe('Ability for users to exit without losing events', () => {
   })
   const _helpers = {
     makeTrackCall: (analytics = ajs, cb?: (...args: any[]) => void) => {
-      analytics.track({ userId: 'foo', event: 'Thing Updated', callback: cb })
+      analytics.track({ userId: 'foo', event: 'Thing Updated' }, cb)
     },
   }
 
@@ -52,22 +52,23 @@ describe('Ability for users to exit without losing events', () => {
 
     test('all callbacks should be called ', async () => {
       const cb = jest.fn()
-      ajs.track({ userId: 'foo', event: 'bar', callback: cb })
+      ajs.track({ userId: 'foo', event: 'bar' }, cb)
       expect(cb).not.toHaveBeenCalled()
       await ajs.closeAndFlush()
       expect(cb).toBeCalled()
     })
 
     test('all async callbacks should be called', async () => {
-      const trackCall = new Promise<CoreContext>((resolve) =>
-        ajs.track({
-          userId: 'abc',
-          event: 'def',
-          callback: (ctx) => {
-            return sleep(100).then(() => resolve(ctx))
+      const trackCall = new Promise<CoreContext>((resolve) => {
+        ajs.track(
+          {
+            userId: 'abc',
+            event: 'def',
           },
-        })
-      )
+          (_, ctx) => sleep(200).then(() => resolve(ctx!))
+        )
+      })
+
       const res = await Promise.race([ajs.closeAndFlush(), trackCall])
       expect(res instanceof CoreContext).toBe(true)
     })

packages/node/src/__tests__/graceful-shutdown-integration.test.ts

@@ -62,7 +62,7 @@ describe('Error handling', () => {
       await promise
       throw new Error('fail')
     } catch (err: any) {
-      expect(err.message).toMatch(/fail/)
+      expect(err.reason).toEqual(new Error('[503] Service Unavailable'))
       expect(err.code).toMatch(/delivery_failure/)
     }
   })