From 0df12694b78c69be29b9623eee02d29192c066f0 Mon Sep 17 00:00:00 2001 From: Jason Salaber Date: Tue, 3 Dec 2024 13:04:34 -0500 Subject: [PATCH] fix: fixed bug with rollout when starting with 100 instead of 0 (#1004) --- dev-apps/nextjs/app-router/app/devcycle.tsx | 11 +- .../bucketing/__tests__/bucketing.test.ts | 169 +++++++++++++++++- sdk/nextjs/package.json | 1 + sdk/nextjs/src/pages/bucketing.ts | 3 +- sdk/nextjs/src/server/bucketing.ts | 7 +- sdk/nextjs/src/server/requests.ts | 3 +- yarn.lock | 3 +- 7 files changed, 186 insertions(+), 11 deletions(-) diff --git a/dev-apps/nextjs/app-router/app/devcycle.tsx b/dev-apps/nextjs/app-router/app/devcycle.tsx index 50466e1f6..ce62423d2 100644 --- a/dev-apps/nextjs/app-router/app/devcycle.tsx +++ b/dev-apps/nextjs/app-router/app/devcycle.tsx @@ -9,15 +9,16 @@ const getUserIdentity = async () => { } } -const { getVariableValue, getClientContext } = setupDevCycle( +const { getVariableValue, getClientContext } = setupDevCycle({ + serverSDKKey: process.env.NEXT_PUBLIC_DEVCYCLE_SERVER_SDK_KEY ?? '', // SDK Key. This will be public and sent to the client, so you MUST use the client SDK key. - process.env.NEXT_PUBLIC_DEVCYCLE_CLIENT_SDK_KEY ?? '', + clientSDKKey: process.env.NEXT_PUBLIC_DEVCYCLE_CLIENT_SDK_KEY ?? '', // pass your method for getting the user identity - getUserIdentity, + userGetter: getUserIdentity, // pass any options you want to use for the DevCycle SDK - { + options: { enableStreaming: process.env.NEXT_PUBLIC_ENABLE_STREAMING === '1', }, -) +}) export { getVariableValue, getClientContext, getUserIdentity } diff --git a/lib/shared/bucketing/__tests__/bucketing.test.ts b/lib/shared/bucketing/__tests__/bucketing.test.ts index 76fcc23c7..1733e27cc 100644 --- a/lib/shared/bucketing/__tests__/bucketing.test.ts +++ b/lib/shared/bucketing/__tests__/bucketing.test.ts @@ -1,5 +1,5 @@ /* eslint-disable max-len */ -import { Audience, FeatureType, Rollout } from '@devcycle/types' +import { Audience, FeatureType, PublicRollout, Rollout } from '@devcycle/types' import { generateBoundedHashes, decideTargetVariation, @@ -1415,6 +1415,173 @@ describe('Rollout Logic', () => { doesUserPassRollout({ rollout, boundedHash: 0.9 }), ).toBeFalsy() }) + + it('should handle stepped rollout with 0% start and 100% later stage', () => { + const rollout: PublicRollout = { + type: 'stepped', + startDate: new Date( + new Date().getTime() - 1000 * 60 * 60 * 24 * 7, + ), + startPercentage: 0, + stages: [ + { + type: 'discrete', + date: new Date( + new Date().getTime() + 1000 * 60 * 60 * 24 * 7, + ), + percentage: 1, + }, + ], + } + + // Before next stage - should be 0% + jest.useFakeTimers().setSystemTime(new Date()) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeFalsy() + } + + // After 100% stage - should pass all users + jest.useFakeTimers().setSystemTime( + new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * 8), + ) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeTruthy() + } + + jest.useRealTimers() + }) + + it('should handle stepped rollout with 50% start and 100% later stage', () => { + const rollout: PublicRollout = { + type: 'stepped', + startDate: new Date( + new Date().getTime() - 1000 * 60 * 60 * 24 * 7, + ), + startPercentage: 0.5, + stages: [ + { + type: 'discrete', + date: new Date( + new Date().getTime() + 1000 * 60 * 60 * 24 * 7, + ), + percentage: 1, + }, + ], + } + + // Before next stage - should be 50% + jest.useFakeTimers().setSystemTime(new Date()) + expect( + doesUserPassRollout({ rollout, boundedHash: 0.49 }), + ).toBeTruthy() + expect( + doesUserPassRollout({ rollout, boundedHash: 0.51 }), + ).toBeFalsy() + + // After 100% stage - should pass all users + jest.useFakeTimers().setSystemTime( + new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * 8), + ) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeTruthy() + } + + jest.useRealTimers() + }) + + it('should handle stepped rollout with 100% start and 0% later stage', () => { + const rollout: PublicRollout = { + type: 'stepped', + startDate: new Date( + new Date().getTime() - 1000 * 60 * 60 * 24 * 7, + ), + startPercentage: 1, + stages: [ + { + type: 'discrete', + date: new Date( + new Date().getTime() + 1000 * 60 * 60 * 24 * 7, + ), + percentage: 0, + }, + ], + } + + // Before next stage - should be 100% + jest.useFakeTimers().setSystemTime(new Date()) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeTruthy() + } + + // After 0% stage - should fail all users + jest.useFakeTimers().setSystemTime( + new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * 8), + ) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeFalsy() + } + + jest.useRealTimers() + }) + + it('should handle stepped rollout with a future start date', () => { + const rollout: PublicRollout = { + type: 'stepped', + startDate: new Date( + new Date().getTime() + 1000 * 60 * 60 * 24 * 7, + ), + startPercentage: 1, + stages: [ + { + type: 'discrete', + date: new Date( + new Date().getTime() + 1000 * 60 * 60 * 24 * 14, + ), + percentage: 0, + }, + ], + } + + // Before next stage - should be no one + jest.useFakeTimers().setSystemTime(new Date()) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeFalsy() + } + + // After start date - should pass all users + jest.useFakeTimers().setSystemTime( + new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * 8), + ) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeTruthy() + } + + // After next stage - should be no one + jest.useFakeTimers().setSystemTime( + new Date(new Date().getTime() + 1000 * 60 * 60 * 24 * 15), + ) + for (let i = 0; i < 100; i++) { + expect( + doesUserPassRollout({ rollout, boundedHash: i / 100 }), + ).toBeFalsy() + } + + jest.useRealTimers() + }) }) it('throws when given an empty rollout object', () => { diff --git a/sdk/nextjs/package.json b/sdk/nextjs/package.json index 2a92dbb7c..16a7201fe 100644 --- a/sdk/nextjs/package.json +++ b/sdk/nextjs/package.json @@ -22,6 +22,7 @@ "@devcycle/js-client-sdk": "^1.32.0", "@devcycle/react-client-sdk": "^1.30.0", "@devcycle/types": "^1.19.0", + "class-transformer": "^0.5.1", "hoist-non-react-statics": "^3.3.2", "server-only": "^0.0.1" }, diff --git a/sdk/nextjs/src/pages/bucketing.ts b/sdk/nextjs/src/pages/bucketing.ts index 778d6a31e..7be3b2d42 100644 --- a/sdk/nextjs/src/pages/bucketing.ts +++ b/sdk/nextjs/src/pages/bucketing.ts @@ -2,6 +2,7 @@ import { DevCycleUser, DVCPopulatedUser } from '@devcycle/js-client-sdk' import { generateBucketedConfig } from '@devcycle/bucketing' import { BucketedUserConfig, ConfigBody, ConfigSource } from '@devcycle/types' import { fetchCDNConfig, sdkConfigAPI } from './requests.js' +import { plainToInstance } from 'class-transformer' class CDNConfigSource extends ConfigSource { async getConfig( @@ -18,7 +19,7 @@ class CDNConfigSource extends ConfigSource { throw new Error('Could not fetch config') } return { - config: await configResponse.json(), + config: plainToInstance(ConfigBody, await configResponse.json()), lastModified: configResponse.headers.get('last-modified'), metaData: {}, } diff --git a/sdk/nextjs/src/server/bucketing.ts b/sdk/nextjs/src/server/bucketing.ts index 57453426d..3c00a1b66 100644 --- a/sdk/nextjs/src/server/bucketing.ts +++ b/sdk/nextjs/src/server/bucketing.ts @@ -48,7 +48,10 @@ const generateBucketedConfigCached = cache( return { bucketedConfig: { - ...generateBucketedConfig({ user: populatedUser, config }), + ...generateBucketedConfig({ + user: populatedUser, + config, + }), clientSDKKey, sse: { url: config.sse @@ -73,7 +76,7 @@ class CDNConfigSource extends ConfigSource { obfuscated, ) return { - config: config, + config, lastModified: headers.get('last-modified'), metaData: {}, } diff --git a/sdk/nextjs/src/server/requests.ts b/sdk/nextjs/src/server/requests.ts index 5bb5f61b0..853d580a7 100644 --- a/sdk/nextjs/src/server/requests.ts +++ b/sdk/nextjs/src/server/requests.ts @@ -2,6 +2,7 @@ import { DVCPopulatedUser } from '@devcycle/js-client-sdk' import { serializeUserSearchParams } from '../common/serializeUser' import { cache } from 'react' import { BucketedUserConfig, ConfigBody } from '@devcycle/types' +import { plainToInstance } from 'class-transformer' const getFetchUrl = (sdkKey: string, obfuscated: boolean) => `https://config-cdn.devcycle.com/config/v2/server/bootstrap/${ @@ -30,7 +31,7 @@ export const fetchCDNConfig = cache( throw new Error('Could not fetch config: ' + responseText) } return { - config: (await response.json()) as ConfigBody, + config: plainToInstance(ConfigBody, await response.json()), headers: response.headers, } }, diff --git a/yarn.lock b/yarn.lock index 35de14abc..3fff47ef4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4708,6 +4708,7 @@ __metadata: "@devcycle/js-client-sdk": ^1.32.0 "@devcycle/react-client-sdk": ^1.30.0 "@devcycle/types": ^1.19.0 + class-transformer: ^0.5.1 hoist-non-react-statics: ^3.3.2 server-only: ^0.0.1 languageName: unknown @@ -13469,7 +13470,7 @@ __metadata: languageName: node linkType: hard -"class-transformer@npm:0.5.1": +"class-transformer@npm:0.5.1, class-transformer@npm:^0.5.1": version: 0.5.1 resolution: "class-transformer@npm:0.5.1" checksum: f191c8b4cc4239990f5efdd790cabdd852c243ed66248e39f6616a349c910c6eed2d9fd1fbf7ee6ea89f69b4f1d0b493b27347fe0cd0ae26b47c3745a805b6d3