From 1b9a40c918e39708d5ce3d6e6738d9168d9a9c4d Mon Sep 17 00:00:00 2001 From: Seth Silesky Date: Thu, 4 Aug 2022 16:15:09 -0500 Subject: [PATCH] Do not allow the user method to change its return types over its lifecycle add unit tests --- .changeset/slimy-worms-fail.md | 5 ++ .../analytics-pre-init.integration.test.ts | 30 +++++++++-- .../typedef-tests/analytics-browser.ts | 9 +++- .../src/core/buffer/__tests__/index.test.ts | 52 +++++++++++++++++++ packages/browser/src/core/buffer/index.ts | 3 +- .../src/test-helpers/type-assertions.ts | 8 +-- 6 files changed, 94 insertions(+), 13 deletions(-) create mode 100644 .changeset/slimy-worms-fail.md diff --git a/.changeset/slimy-worms-fail.md b/.changeset/slimy-worms-fail.md new file mode 100644 index 000000000..2f48ba58d --- /dev/null +++ b/.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. \ No newline at end of file diff --git a/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts b/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts index 6b5be1810..1b92b2547 100644 --- a/packages/browser/src/browser/__tests__/analytics-pre-init.integration.test.ts +++ b/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 () => { diff --git a/packages/browser/src/browser/__tests__/typedef-tests/analytics-browser.ts b/packages/browser/src/browser/__tests__/typedef-tests/analytics-browser.ts index 32df73f8b..298f5a384 100644 --- a/packages/browser/src/browser/__tests__/typedef-tests/analytics-browser.ts +++ b/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>(grpResult) } }, + 'User should have the correct type': () => { + const ajs = AnalyticsBrowser.load({ writeKey: 'foo' }) + { + const grpResult = ajs.user() + assertIs>(grpResult) + } + }, } diff --git a/packages/browser/src/core/buffer/__tests__/index.test.ts b/packages/browser/src/core/buffer/__tests__/index.test.ts index 6088fd1fa..04950e766 100644 --- a/packages/browser/src/core/buffer/__tests__/index.test.ts +++ b/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' }) diff --git a/packages/browser/src/core/buffer/index.ts b/packages/browser/src/core/buffer/index.ts index 0d1bc3a36..d7b42edb0 100644 --- a/packages/browser/src/core/buffer/index.ts +++ b/packages/browser/src/core/buffer/index.ts @@ -248,7 +248,8 @@ export class AnalyticsBuffered ...args: Parameters ): Promise> => { 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) => { diff --git a/packages/browser/src/test-helpers/type-assertions.ts b/packages/browser/src/test-helpers/type-assertions.ts index 8557ee58c..b25a09e7e 100644 --- a/packages/browser/src/test-helpers/type-assertions.ts +++ b/packages/browser/src/test-helpers/type-assertions.ts @@ -5,11 +5,7 @@ type NotUnknown = unknown extends T ? never : T type NotTopType = NotAny & NotUnknown // this is not meant to be run, just for type tests -export function assertNotAny(val: NotTopType) { - console.log(val) -} +export function assertNotAny(_val: NotTopType) {} // this is not meant to be run, just for type tests -export function assertIs(val: T) { - console.log(val) -} +export function assertIs(_val: T) {}