Do not allow the user method to change its return types over its lifecycle (#567)

add unit tests

Author: silesky

.changeset/slimy-worms-fail.md

@@ -0,0 +1,5 @@
+---
+'@segment/analytics-next': patch
+---
+
+Do not allow the "user" method to change its return types over its lifecycle. We should always return a promise for wrapped methods in AnalyticsBrowser, regardless if the underlying Analytics method is sync or async.

packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts

@@ -5,6 +5,7 @@ import { Context } from '../../core/context'
 import * as Factory from '../../test-helpers/factories'
 import { sleep } from '../../test-helpers/sleep'
 import { setGlobalCDNUrl } from '../../lib/parse-cdn'
+import { User } from '../../core/user'
 
 jest.mock('unfetch')
 
@@ -59,19 +60,38 @@ describe('Pre-initialization', () => {
       expect(trackSpy).toBeCalledTimes(1)
     })
 
-    test('"return types should not change over the lifecycle for ordinary methods', async () => {
+    test('"return types should not change over the lifecycle for async methods', async () => {
       const ajsBrowser = AnalyticsBrowser.load({ writeKey })
 
       const trackCtxPromise1 = ajsBrowser.track('foo', { name: 'john' })
       expect(trackCtxPromise1).toBeInstanceOf(Promise)
-      const ctx1 = await trackCtxPromise1
-      expect(ctx1).toBeInstanceOf(Context)
+      await ajsBrowser
 
       // loaded
       const trackCtxPromise2 = ajsBrowser.track('foo', { name: 'john' })
       expect(trackCtxPromise2).toBeInstanceOf(Promise)
-      const ctx2 = await trackCtxPromise2
-      expect(ctx2).toBeInstanceOf(Context)
+
+      expect(await trackCtxPromise1).toBeInstanceOf(Context)
+      expect(await trackCtxPromise2).toBeInstanceOf(Context)
+    })
+
+    test('return types should not change over lifecycle for sync methods', async () => {
+      const ajsBrowser = AnalyticsBrowser.load({ writeKey })
+      const user = ajsBrowser.user()
+      expect(user).toBeInstanceOf(Promise)
+      await ajsBrowser
+
+      // loaded
+      const user2 = ajsBrowser.user()
+      expect(user2).toBeInstanceOf(Promise)
+
+      expect(await user).toBeInstanceOf(User)
+      expect(await user2).toBeInstanceOf(User)
+    })
+
+    test('version should return version', async () => {
+      const ajsBrowser = AnalyticsBrowser.load({ writeKey })
+      expect(typeof ajsBrowser.VERSION).toBe('string')
     })
 
     test('If a user sends multiple events, all of those event gets flushed', async () => {

packages/browser/src/browser/__tests__/typedef-tests/analytics-browser.ts

@@ -2,7 +2,7 @@ import { Analytics } from '@/core/analytics'
 import { Context } from '@/core/context'
 import { AnalyticsBrowser } from '@/browser'
 import { assertNotAny, assertIs } from '@/test-helpers/type-assertions'
-import { Group } from '../../../core/user'
+import { Group, User } from '../../../core/user'
 
 /**
  * These are general typescript definition tests;
@@ -67,4 +67,11 @@ export default {
       assertIs<Promise<Context>>(grpResult)
     }
   },
+  'User should have the correct type': () => {
+    const ajs = AnalyticsBrowser.load({ writeKey: 'foo' })
+    {
+      const grpResult = ajs.user()
+      assertIs<Promise<User>>(grpResult)
+    }
+  },
 }

packages/browser/src/core/buffer/__tests__/index.test.ts

@@ -8,6 +8,7 @@ import {
 import { Analytics } from '../../analytics'
 import { Context } from '../../context'
 import { sleep } from '@/test-helpers/sleep'
+import { User } from '../../user'
 
 describe('PreInitMethodCallBuffer', () => {
   describe('push', () => {
@@ -99,6 +100,57 @@ describe('AnalyticsBuffered', () => {
       expect(context).toEqual(ctx)
     })
 
+    it('should wrap async methods in a promise', async () => {
+      const ajs = new Analytics({ writeKey: 'foo' })
+      const ctx = new Context({ type: 'track' })
+
+      ajs.track = () => Promise.resolve(ctx)
+
+      const buffered = new AnalyticsBuffered((buffer) => {
+        return new Promise((resolve) =>
+          setTimeout(() => {
+            flushAnalyticsCallsInNewTask(ajs, buffer)
+            resolve([ajs, ctx])
+          }, 0)
+        )
+      })
+      const result = buffered.track('foo')
+      expect(result).toBeInstanceOf(Promise)
+
+      await buffered
+
+      const result2 = buffered.track('bar')
+      expect(result2).toBeInstanceOf(Promise)
+
+      expect(await result).toBeInstanceOf(Context)
+      expect(await result2).toBeInstanceOf(Context)
+    })
+
+    it('should wrap synchronous methods in a promise', async () => {
+      const ajs = new Analytics({ writeKey: 'foo' })
+      ajs.user = () => new User()
+      const ctx = new Context({ type: 'track' })
+
+      const buffered = new AnalyticsBuffered((buffer) => {
+        return new Promise((resolve) =>
+          setTimeout(() => {
+            flushAnalyticsCallsInNewTask(ajs, buffer)
+            resolve([ajs, ctx])
+          }, 0)
+        )
+      })
+      const result = buffered.user()
+      expect(result).toBeInstanceOf(Promise)
+
+      await buffered
+
+      const result2 = buffered.user()
+      expect(result2).toBeInstanceOf(Promise)
+
+      expect(await result).toBeInstanceOf(User)
+      expect(await result2).toBeInstanceOf(User)
+    })
+
     describe('the "this" value of proxied analytics methods', () => {
       test('should be the ajs instance for non-chainable methods (that return a promise)', async () => {
         const ajs = new Analytics({ writeKey: 'foo' })

packages/browser/src/core/buffer/index.ts

@@ -248,7 +248,8 @@ export class AnalyticsBuffered
       ...args: Parameters<Analytics[T]>
     ): Promise<ReturnTypeUnwrap<Analytics[T]>> => {
       if (this.instance) {
-        return (this.instance[methodName] as Function)(...args)
+        const result = (this.instance[methodName] as Function)(...args)
+        return Promise.resolve(result)
       }
 
       return new Promise((resolve, reject) => {

packages/browser/src/test-helpers/type-assertions.ts

@@ -5,11 +5,7 @@ type NotUnknown<T> = unknown extends T ? never : T
 type NotTopType<T> = NotAny<T> & NotUnknown<T>
 
 // this is not meant to be run, just for type tests
-export function assertNotAny<T>(val: NotTopType<T>) {
-  console.log(val)
-}
+export function assertNotAny<T>(_val: NotTopType<T>) {}
 
 // this is not meant to be run, just for type tests
-export function assertIs<T extends SomeType, SomeType = any>(val: T) {
-  console.log(val)
-}
+export function assertIs<T extends SomeType, SomeType = any>(_val: T) {}