improve Segment.io retries (#714)

.changeset/clean-lizards-nail.md

@@ -0,0 +1,5 @@
+---
+'@segment/analytics-next': patch
+---
+
+Improves Segment.io retries to include exponential backoff and work across page loads.

packages/browser-integration-tests/package.json

@@ -20,6 +20,7 @@
     "@playwright/test": "^1.28.1",
     "@segment/analytics-next": "workspace:^",
     "http-server": "14.1.1",
-    "nock": "^13.2.9"
+    "nock": "^13.2.9",
+    "tslib": "^2.4.1"
   }
 }

packages/browser-integration-tests/src/segment-retries.test.ts

@@ -1,4 +1,4 @@
-import { test, expect } from '@playwright/test'
+import { Request, test, expect } from '@playwright/test'
 import { SettingsBuilder } from './fixtures/settings'
 import { standaloneMock } from './helpers/standalone-mock'
 
@@ -25,7 +25,9 @@ test.describe('Standalone tests', () => {
       )
     })
 
-    test.skip('supports retries on page navigation', async ({ page }) => {
+    test('supports retrying failed requests on page navigation', async ({
+      page,
+    }) => {
       // Load analytics.js
       await page.goto('/standalone.html')
       await page.evaluate(() => window.analytics.load('fake-key'))
@@ -40,16 +42,16 @@ test.describe('Standalone tests', () => {
           times: 1,
         }
       )
-      const requestFailure = new Promise((resolve) => {
-        page.once('requestfailed', resolve)
+      const requestFailure = new Promise<Record<string, any>>((resolve) => {
+        page.once('requestfailed', (request) => resolve(request.postDataJSON()))
       })
 
       // trigger an event
       await page.evaluate(() => {
         void window.analytics.track('test event')
       })
 
-      await requestFailure
+      const { messageId } = await requestFailure
       await page.reload()
 
       // load analytics.js again and wait for a new request.
@@ -59,6 +61,55 @@ test.describe('Standalone tests', () => {
       ])
 
       expect(request.method()).toBe('POST')
+      expect(request.postDataJSON().messageId).toBe(messageId)
+    })
+
+    test('supports retrying in-flight requests on page navigation', async ({
+      page,
+    }) => {
+      // Load analytics.js
+      await page.goto('/standalone.html')
+      await page.evaluate(() => window.analytics.load('fake-key'))
+
+      // blackhole the request so that it stays in-flight when we reload the page
+      await page.route(
+        'https://api.segment.io/v1/t',
+        async () => {
+          // do nothing
+        },
+        {
+          times: 1,
+        }
+      )
+
+      // Detect when we've seen a track request initiated by the browser
+      const requestSent = new Promise<Record<string, any>>((resolve) => {
+        const onRequest: (req: Request) => void = (req) => {
+          if (req.url() === 'https://api.segment.io/v1/t') {
+            page.off('request', onRequest)
+            resolve(req.postDataJSON())
+          }
+        }
+
+        page.on('request', onRequest)
+      })
+
+      // trigger an event
+      await page.evaluate(() => {
+        void window.analytics.track('test event')
+      })
+
+      const { messageId } = await requestSent
+      await page.reload()
+
+      // load analytics.js again and wait for a new request.
+      const [request] = await Promise.all([
+        page.waitForRequest('https://api.segment.io/v1/t'),
+        page.evaluate(() => window.analytics.load('fake-key')),
+      ])
+
+      expect(request.method()).toBe('POST')
+      expect(request.postDataJSON().messageId).toBe(messageId)
     })
   })
 })

packages/browser-integration-tests/src/shims.d.ts

@@ -0,0 +1,7 @@
+import type { AnalyticsSnippet } from '@segment/analytics-next'
+
+declare global {
+  interface Window {
+    analytics: AnalyticsSnippet
+  }
+}

packages/browser/package.json

@@ -56,7 +56,7 @@
     "js-cookie": "3.0.1",
     "node-fetch": "^2.6.7",
     "spark-md5": "^3.0.1",
-    "tslib": "^2.4.0",
+    "tslib": "^2.4.1",
     "unfetch": "^4.1.0"
   },
   "devDependencies": {

packages/browser/src/lib/__tests__/on-page-leave.test.ts

@@ -2,7 +2,8 @@ import { onPageLeave } from '../on-page-leave'
 
 const helpers = {
   dispatchEvent(event: 'pagehide' | 'visibilitychange') {
-    document.dispatchEvent(new Event(event))
+    const target = event === 'pagehide' ? window : document
+    target.dispatchEvent(new Event(event))
   },
   setVisibilityState(state: DocumentVisibilityState) {
     Object.defineProperty(document, 'visibilityState', {

packages/browser/src/lib/on-page-leave.ts

@@ -10,13 +10,13 @@
 export const onPageLeave = (cb: (...args: unknown[]) => void) => {
   let unloaded = false // prevents double firing if both are supported
 
-  // using document instead of window because of bug affecting browsers before safari 14 (detail in footnotes https://caniuse.com/?search=visibilitychange)
-  document.addEventListener('pagehide', () => {
+  window.addEventListener('pagehide', () => {
     if (unloaded) return
     unloaded = true
     cb()
   })
 
+  // using document instead of window because of bug affecting browsers before safari 14 (detail in footnotes https://caniuse.com/?search=visibilitychange)
   document.addEventListener('visibilitychange', () => {
     if (document.visibilityState == 'hidden') {
       if (unloaded) return

packages/browser/src/lib/priority-queue/__tests__/persisted.test.ts

@@ -41,7 +41,7 @@ describe('Persisted Priority Queue', () => {
     let onUnload: any = jest.fn()
 
     jest
-      .spyOn(document, 'addEventListener')
+      .spyOn(window, 'addEventListener')
       .mockImplementation((evt, handler) => {
         if (evt === 'pagehide') {
           onUnload = handler
@@ -102,7 +102,7 @@ describe('Persisted Priority Queue', () => {
       let onUnload: any = jest.fn()
 
       jest
-        .spyOn(document, 'addEventListener')
+        .spyOn(window, 'addEventListener')
         .mockImplementation((evt, handler) => {
           if (evt === 'pagehide') {
             onUnload = handler
@@ -200,7 +200,7 @@ describe('Persisted Priority Queue', () => {
       const onUnloadFunctions: any[] = []
 
       jest
-        .spyOn(document, 'addEventListener')
+        .spyOn(window, 'addEventListener')
         .mockImplementation((evt, handler) => {
           if (evt === 'pagehide') {
             onUnloadFunctions.push(handler)

packages/browser/src/lib/priority-queue/persisted.ts

@@ -109,7 +109,7 @@ export class PersistedPriorityQueue extends PriorityQueue<Context> {
       }
     })
 
-    document.addEventListener('pagehide', () => {
+    window.addEventListener('pagehide', () => {
       // we deliberately want to use the less powerful 'pagehide' API to only persist on events where the analytics instance gets destroyed, and not on tab away.
       if (this.todo > 0) {
         const items = [...this.queue, ...this.future]

packages/browser/src/plugins/segmentio/__tests__/batched-dispatcher.test.ts

@@ -224,7 +224,7 @@ describe('Batching', () => {
 
       expect(fetch).not.toHaveBeenCalled()
 
-      document.dispatchEvent(new Event('pagehide'))
+      window.dispatchEvent(new Event('pagehide'))
 
       expect(fetch).toHaveBeenCalledTimes(1)
 
@@ -253,7 +253,7 @@ describe('Batching', () => {
 
       expect(fetch).not.toHaveBeenCalled()
 
-      document.dispatchEvent(new Event('pagehide'))
+      window.dispatchEvent(new Event('pagehide'))
 
       expect(fetch).toHaveBeenCalledTimes(2)
     })

packages/browser/src/plugins/segmentio/index.ts

@@ -50,12 +50,21 @@ export function segmentio(
   settings?: SegmentioSettings,
   integrations?: LegacySettings['integrations']
 ): Plugin {
+  // Attach `pagehide` before buffer is created so that inflight events are added
+  // to the buffer before the buffer persists events in its own `pagehide` handler.
+  window.addEventListener('pagehide', () => {
+    buffer.push(...Array.from(inflightEvents))
+    inflightEvents.clear()
+  })
+
   const buffer = analytics.options.disableClientPersistence
     ? new PriorityQueue<Context>(analytics.queue.queue.maxAttempts, [])
     : new PersistedPriorityQueue(
         analytics.queue.queue.maxAttempts,
         `dest-Segment.io`
       )
+
+  const inflightEvents = new Set<Context>()
   const flushing = false
 
   const apiHost = settings?.apiHost ?? 'api.segment.io/v1'
@@ -75,6 +84,8 @@ export function segmentio(
       return ctx
     }
 
+    inflightEvents.add(ctx)
+
     const path = ctx.event.type.charAt(0)
     let json = toFacade(ctx.event).json()
 
@@ -92,14 +103,15 @@ export function segmentio(
         normalize(analytics, json, settings, integrations)
       )
       .then(() => ctx)
-      .catch((err) => {
-        if (err.type === 'error' || err.message === 'Failed to fetch') {
-          buffer.push(ctx)
-          // eslint-disable-next-line @typescript-eslint/no-use-before-define
-          scheduleFlush(flushing, buffer, segmentio, scheduleFlush)
-        }
+      .catch(() => {
+        buffer.pushWithBackoff(ctx)
+        // eslint-disable-next-line @typescript-eslint/no-use-before-define
+        scheduleFlush(flushing, buffer, segmentio, scheduleFlush)
         return ctx
       })
+      .finally(() => {
+        inflightEvents.delete(ctx)
+      })
   }
 
   const segmentio: Plugin = {
@@ -115,5 +127,11 @@ export function segmentio(
     group: send,
   }
 
+  // Buffer may already have items if they were previously stored in localStorage.
+  // Start flushing them immediately.
+  if (buffer.todo) {
+    scheduleFlush(flushing, buffer, segmentio, scheduleFlush)
+  }
+
   return segmentio
 }

packages/core/package.json

@@ -34,6 +34,6 @@
   "dependencies": {
     "@lukeed/uuid": "^2.0.0",
     "dset": "^3.1.2",
-    "tslib": "^2.4.0"
+    "tslib": "^2.4.1"
   }
 }

packages/node/package.json

@@ -36,7 +36,7 @@
     "@lukeed/uuid": "^2.0.0",
     "@segment/analytics-core": "1.1.5",
     "node-fetch": "^2.6.7",
-    "tslib": "^2.4.0"
+    "tslib": "^2.4.1"
   },
   "devDependencies": {
     "@internal/config": "0.0.0",

packages/test-helpers/package.json

@@ -19,7 +19,7 @@
     "concurrently": "yarn run -T concurrently"
   },
   "dependencies": {
-    "tslib": "^2.4.0"
+    "tslib": "^2.4.1"
   },
   "packageManager": "yarn@3.2.1"
 }

yarn.lock

@@ -1194,6 +1194,7 @@ __metadata:
     "@segment/analytics-next": "workspace:^"
     http-server: 14.1.1
     nock: ^13.2.9
+    tslib: ^2.4.1
   languageName: unknown
   linkType: soft
 
@@ -1232,7 +1233,7 @@ __metadata:
   version: 0.0.0-use.local
   resolution: "@internal/test-helpers@workspace:packages/test-helpers"
   dependencies:
-    tslib: ^2.4.0
+    tslib: ^2.4.1
   languageName: unknown
   linkType: soft
 
@@ -1945,7 +1946,7 @@ __metadata:
   dependencies:
     "@lukeed/uuid": ^2.0.0
     dset: ^3.1.2
-    tslib: ^2.4.0
+    tslib: ^2.4.1
   languageName: unknown
   linkType: soft
 
@@ -1999,7 +2000,7 @@ __metadata:
     terser-webpack-plugin: ^5.1.4
     ts-loader: ^9.1.1
     ts-node: ^10.8.0
-    tslib: ^2.4.0
+    tslib: ^2.4.1
     unfetch: ^4.1.0
     webpack: ^5.36.1
     webpack-bundle-analyzer: ^4.4.2
@@ -2016,7 +2017,7 @@ __metadata:
     "@segment/analytics-core": 1.1.5
     "@types/node": ^14
     node-fetch: ^2.6.7
-    tslib: ^2.4.0
+    tslib: ^2.4.1
   languageName: unknown
   linkType: soft
 
@@ -13795,6 +13796,13 @@ __metadata:
   languageName: node
   linkType: hard
 
+"tslib@npm:^2.4.1":
+  version: 2.4.1
+  resolution: "tslib@npm:2.4.1"
+  checksum: 19480d6e0313292bd6505d4efe096a6b31c70e21cf08b5febf4da62e95c265c8f571f7b36fcc3d1a17e068032f59c269fab3459d6cd3ed6949eafecf64315fca
+  languageName: node
+  linkType: hard
+
 "tsutils@npm:^3.21.0":
   version: 3.21.0
   resolution: "tsutils@npm:3.21.0"