From 439623df1893dfb430b297bf8f1715963aab9590 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 30 Apr 2026 10:45:59 +0100 Subject: [PATCH 01/25] fix: code quality, type safety, error handling, and test coverage (268/268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Squashed from the following 18 commits (oldest → newest): - 3f6e102 fix: improve code quality, type safety, and test coverage (268/268 passing) - b7c5c1a fix: correct equality and in operators for type coercion edge cases - 9cdc91e docs: restructure README to match standard SDK documentation structure - 589f8c9 feat: add public getSDK() and getOptions() methods to Context - 3d35b12 fix(js): return safe defaults from read methods when context not ready or finalized - bcaf531 feat: cross-SDK consistency fixes — all 201 scenarios passing - d455fe6 fix: address coderabbit review issues - 469ebe1 fix: improve error handling, compatibility, and observability - 8b98baa fix: restore throw-on-finalized behavior for all context methods - f7a27a1 fix: address coderabbit review comments - a8043e0 fix: correct brand name casing from ABSmartly to ABsmartly - d590218 fix: address coderabbit review issues - 43916a2 fix: ready() now correctly returns false on failed init - c3c58b1 fix: update ready() example to use boolean return value - 414dc5a fix: clarify ready() is a wait signal, not a gate - 97b515b fix: ready() always resolves true to prevent misuse as gate - 7346806 fix: revert InOperator argument swap — haystack,needle order is correct - 951848b fix: restore synchronous flush reset with retry on publish failure Pre-squash branch tip preserved at archive/fix-all-tests-passing-pre-squash. --- .gitignore | 3 +- README.md | 513 ++++++++++++++------ src/__tests__/context.test.js | 310 +++++++++++- src/__tests__/fixes.test.js | 475 ++++++++++++++++++ src/__tests__/jsonexpr/operators/eq.test.js | 6 +- src/abort-controller-shim.ts | 13 +- src/client.ts | 33 +- src/context.ts | 264 +++++++--- src/fetch.ts | 68 ++- src/index.ts | 4 +- src/jsonexpr/operators/eq.ts | 6 + src/jsonexpr/operators/match.ts | 41 +- src/matcher.ts | 16 +- src/provider.ts | 2 +- src/publisher.ts | 4 +- src/sdk.ts | 81 ++-- src/utils.ts | 31 +- 17 files changed, 1533 insertions(+), 337 deletions(-) create mode 100644 src/__tests__/fixes.test.js diff --git a/.gitignore b/.gitignore index 710fa5a..a659a0a 100644 --- a/.gitignore +++ b/.gitignore @@ -8,5 +8,6 @@ src/js/* !src/js/__tests__ types js -.claude/worktrees +.claude/ +.DS_Store src/version.ts diff --git a/README.md b/README.md index 043607c..1cf3de2 100644 --- a/README.md +++ b/README.md @@ -1,52 +1,71 @@ -# A/B Smartly SDK [![npm version](https://badge.fury.io/js/%40absmartly%2Fjavascript-sdk.svg)](https://badge.fury.io/js/%40absmartly%2Fjavascript-sdk) +# ABsmartly JavaScript SDK [![npm version](https://badge.fury.io/js/%40absmartly%2Fjavascript-sdk.svg)](https://badge.fury.io/js/%40absmartly%2Fjavascript-sdk) -A/B Smartly - JavaScript SDK +A/B Smartly - JavaScript SDK. This is the official isomorphic JavaScript SDK for the [A/B Smartly](https://www.absmartly.com/) A/B testing platform, compatible with both Node.js and browser environments. ## Compatibility -The A/B Smartly Javascript SDK is an isomorphic library for Node.js (CommonJS and ES6) and browsers (UMD). +The A/B Smartly JavaScript SDK is an isomorphic library for Node.js (CommonJS and ES6) and browsers (UMD). -It's supported on Node.js version 6.x and npm 3.x or later. +- **Node.js**: Version 6.x and npm 3.x or later +- **Browsers**: IE 10+ and all modern browsers (Chrome, Firefox, Safari, Edge) -It's supported on IE 10+ and all the other major browsers. - -**Note**: IE 10 does not natively support Promises. -If you target IE 10, you must include a polyfill like [es6-promise](https://www.npmjs.com/package/es6-promise) or [rsvp](https://www.npmjs.com/package/rsvp). +**Note**: IE 10 does not natively support Promises. If you target IE 10, you must include a polyfill like [es6-promise](https://www.npmjs.com/package/es6-promise) or [rsvp](https://www.npmjs.com/package/rsvp). ## Installation -#### npm +### npm ```shell npm install @absmartly/javascript-sdk --save ``` -#### Import in your Javascript application +### Import in your JavaScript application + ```javascript -const absmartly = require('@absmartly/javascript-sdk'); +const absmartly = require("@absmartly/javascript-sdk"); + // OR with ES6 modules: -import absmartly from '@absmartly/javascript-sdk'; +import absmartly from "@absmartly/javascript-sdk"; ``` +### Directly in the browser -#### Directly in the browser You can include an optimized and pre-built package directly in your HTML code through [unpkg.com](https://www.unpkg.com). Simply add the following code to your `head` section to include the latest published version. + ```html - + ``` +### Security Warning: Client-Side Usage + +**IMPORTANT:** This SDK exposes your API key when used directly in browser environments. API keys should never be embedded in client-side code as they can be extracted from browser bundles or network requests. + +**Recommended Architecture:** +- **DO NOT** use this SDK directly in the browser with your API key +- **DO** use this SDK in Node.js server-side applications +- **DO** create a server-side proxy endpoint that fetches context data on behalf of your frontend +- **DO** use short-lived, per-session tokens instead of API keys for client-side requests + +```text +Browser --> Your Server (with API key) --> ABsmartly API + session token only +``` + +For production browser applications, contact A/B Smartly support for client-side SDK recommendations. + ## Getting Started -Please follow the [installation](#installation) instructions before trying the following code: +Please follow the [installation](#installation) instructions before trying the following code. + +### Initialization + +This example assumes an API Key, an Application, and an Environment have been created in the A/B Smartly web console. -#### Initialization -This example assumes an Api Key, an Application, and an Environment have been created in the A/B Smartly web console. ```javascript -// somewhere in your application initialization code const sdk = new absmartly.SDK({ - endpoint: 'https://sandbox.absmartly.io/v1', + endpoint: "https://your-company.absmartly.io/v1", apiKey: process.env.ABSMARTLY_API_KEY, environment: process.env.NODE_ENV, application: process.env.APPLICATION_NAME, @@ -63,90 +82,123 @@ const sdk = new absmartly.SDK({ }); ``` +**SDK Options** + +| Option | Type | Required? | Default | Description | +| :----------- | :--------- | :-------: | :-----: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| endpoint | `string` | ✅ | `null` | The URL to your API endpoint. Most commonly `"your-company.absmartly.io"` | +| apiKey | `string` | ✅ | `null` | Your API key which can be found on the Web Console. | +| environment | `string` | ✅ | `null` | The environment of the platform where the SDK is installed. Environments are created on the Web Console and should match the available environments in your infrastructure. | +| application | `string` | ✅ | `null` | The name of the application where the SDK is installed. Applications are created on the Web Console and should match the applications where your experiments will be running.| +| retries | `number` | ❌ | `5` | Number of retry attempts for failed HTTP requests. | +| timeout | `number` | ❌ | `3000` | HTTP request timeout in milliseconds. | +| eventLogger | `function` | ❌ | `null` | Callback to handle SDK events (ready, exposure, goal, etc.) | + #### Creating a new Context with raw promises ```javascript -// define a new context request const request = { units: { - session_id: '5ebf06d8cb5d8137290c4abb64155584fbdb64d8', + session_id: "5ebf06d8cb5d8137290c4abb64155584fbdb64d8", }, }; -// create context with raw promises const context = sdk.createContext(request); -context.ready().then((response) => { - console.log("ABSmartly Context ready!") -}).catch((error) => { - console.log(error); +context.ready().then(() => { + console.log("ABsmartly Context ready!"); + if (context.isFailed()) { + console.error("ABsmartly Context failed:", context.readyError()); + } + + const treatment = context.treatment("exp_test"); }); ``` -#### Creating a new Context with async/await +### With async/await + ```javascript -// define a new context request const request = { units: { - session_id: '5ebf06d8cb5d8137290c4abb64155584fbdb64d8', + session_id: "5ebf06d8cb5d8137290c4abb64155584fbdb64d8", }, }; -// create context with raw promises const context = sdk.createContext(request); -try { - await context.ready(); - console.log("ABSmartly Context ready!") -} catch (error) { - console.log(error); +await context.ready(); +if (context.isFailed()) { + console.error("ABsmartly Context failed:", context.readyError()); } + +const treatment = context.treatment("exp_test"); ``` -#### Creating a new Context with pre-fetched data -When doing full-stack experimentation with A/B Smartly, we recommend creating a context only once on the server-side. -Creating a context involves a round-trip to the A/B Smartly event collector. -We can avoid repeating the round-trip on the client-side by sending the server-side data embedded in the first document, for example, by rendering it on the template. -Then we can initialize the A/B Smartly context on the client-side directly with it. +### With Pre-fetched Data + +When doing full-stack experimentation with A/B Smartly, we recommend creating a context only once on the server-side. Creating a context involves a round-trip to the A/B Smartly event collector. We can avoid repeating the round-trip on the client-side by sending the server-side data embedded in the first document, for example, by rendering it on the template. Then we can initialize the A/B Smartly context on the client-side directly with it. ```html - - - -``` - -#### Setting extra units for a context -You can add additional units to a context by calling the `unit()` or the `units()` method. -This method may be used for example, when a user logs in to your application, and you want to use the new unit type to the context. -Please note that **you cannot override an already set unit type** as that would be a change of identity, and will throw an exception. In this case, you must create a new context instead. -The `unit()` and `units()` methods can be called before the context is ready. + + + +``` + +### Refreshing the Context with Fresh Experiment Data + +For long-running single-page-applications (SPA), the context is usually created once when the application is first reached. However, any experiments being tracked in your production code, but started after the context was created, will not be triggered. To mitigate this, we can use the `refreshInterval` option when creating the context. ```javascript -context.unit('db_user_id', 1000013); +const request = { + units: { + session_id: "5ebf06d8cb5d8137290c4abb64155584fbdb64d8", + }, +}; -// or -context.units({ - db_user_id: 1000013, +const context = sdk.createContext(request, { + refreshInterval: 5 * 60 * 1000, // 5 minutes }); ``` -#### Setting context attributes -The `attribute()` and `attributes()` methods can be called before the context is ready. +Alternatively, the `refresh()` method can be called manually. The `refresh()` method pulls updated experiment data from the A/B Smartly collector and will trigger recently started experiments when `treatment()` is called again. + ```javascript -context.attribute('user_agent', navigator.userAgent); +setTimeout(async () => { + try { + await context.refresh(); + } catch (error) { + console.error(error); + } +}, 5 * 60 * 1000); +``` -context.attributes({ - customer_age: 'new_customer', +### Setting Extra Units + +You can add additional units to a context by calling the `unit()` or the `units()` method. This is useful when a user logs in to your application and you want to add the new unit type to the context. + +> **Note:** You cannot override an already set unit type as that would be a change of identity. In this case, you must create a new context instead. + +The `unit()` and `units()` methods can be called before the context is ready. + +```javascript +context.unit("db_user_id", 1000013); + +context.units({ + db_user_id: 1000013, }); ``` +## Basic Usage + +### Selecting a Treatment + #### Including system attributes You can opt in to automatically include system attributes (SDK name, SDK version, application, environment, and application version) in every publish payload. These are sent as context attributes and can be useful for debugging and filtering in the Web Console. @@ -171,29 +223,89 @@ These system attributes are prepended before any user-defined attributes. #### Selecting a treatment ```javascript -if (context.treament("exp_test_experiment") == 0) { +if (context.treatment("exp_test_experiment") === 0) { // user is in control group (variant 0) } else { // user is in treatment group } ``` -#### Tracking a goal achievement -Goals are created in the A/B Smartly web console. +### Treatment Variables + +Variables allow you to configure experiment variations without code changes. + ```javascript -context.track("payment", { item_count: 1, total_amount: 1999.99 }); +const defaultButtonColor = "red"; +const buttonColor = context.variableValue("button.color", defaultButtonColor); ``` -#### Publishing pending data -Sometimes it is necessary to ensure all events have been published to the A/B Smartly collector, before proceeding. -One such case is when the user is about to navigate away right before being exposed to a treatment. -You can explicitly call the `publish()` method, which returns a promise, before navigating away. +### Peek at Treatment Variants + +Although generally not recommended, it is sometimes necessary to peek at a treatment without triggering an exposure. The A/B Smartly SDK provides a `peek()` method for that. + ```javascript -await context.publish().then(() => { - window.location = "https://www.absmartly.com" -}) +if (context.peek("exp_test_experiment") === 0) { + // user is in control group (variant 0) +} else { + // user is in treatment group +} +``` + +#### Peeking at Variable Values + +```javascript +const buttonColor = context.peekVariableValue("button.color", "red"); ``` +### Overriding Treatment Variants + +During development, it is useful to force a treatment for an experiment. This can be achieved with the `override()` and/or `overrides()` methods. The `override()` and `overrides()` methods can be called before the context is ready. + +```javascript +context.override("exp_test_experiment", 1); // force variant 1 + +context.overrides({ + exp_test_experiment: 1, + exp_another_experiment: 0, +}); +``` + +## Advanced + +### Context Attributes + +Attributes are used to pass meta-data about the user and/or the request. They can be used later in the Web Console to create segments or audiences. They can be set using the `attribute()` or `attributes()` methods, before or after the context is ready. + +```javascript +context.attribute("user_agent", navigator.userAgent); + +context.attributes({ + customer_age: "new_customer", +}); +``` + +### Custom Assignments + +Sometimes it may be necessary to override the automatic selection of a variant. For example, if you wish to have your variant chosen based on data from an API call. This can be accomplished using the `customAssignment()` method. + +```javascript +context.customAssignment("exp_test_experiment", 1); + +context.customAssignments({ + exp_test_experiment: 1, +}); +``` + +### Tracking Goals + +Goals are created in the A/B Smartly web console. + +```javascript +context.track("payment", { item_count: 1, total_amount: 1999.99 }); +``` + +### Publishing Pending Data + #### Finalizing The `finalize()` method will ensure all events have been published to the A/B Smartly collector, like `publish()`, and will also "seal" the context, throwing an error if any method that could generate an event is called. ```javascript @@ -219,88 +331,64 @@ const context = sdk.createContext(request, { }); ``` -Alternatively, the `refresh()` method can be called manually. -The `refresh()` method pulls updated experiment data from the A/B Smartly collector and will trigger recently started experiments when `treatment()` is called again. +### Finalizing + +The `finalize()` method will ensure all events have been published to the A/B Smartly collector, like `publish()`, and will also "seal" the context, throwing an error if any method is called after finalization. + ```javascript -setTimeout(async () => { - try { - context.refresh(); - } catch(error) { - console.error(error); - } -}, 5 * 60 * 1000); +await context.finalize(); +window.location = "https://www.absmartly.com"; ``` -#### Using a custom Event Logger -The A/B Smartly SDK can be instantiated with an event logger used for all contexts. -In addition, an event logger can be specified when creating a particular context, in the `createContext` call options. -The example below illustrates this with the implementation of the default event logger, used if none is specified. +### Using a Custom Event Logger + +The A/B Smartly SDK can be instantiated with an event logger used for all contexts. In addition, an event logger can be specified when creating a particular context, in the `createContext` call options. The example below illustrates this with the implementation of the default event logger, used if none is specified. + ```javascript const sdk = new absmartly.SDK({ - endpoint: 'https://sandbox-api.absmartly.com/v1', + endpoint: "https://your-company.absmartly.io/v1", apiKey: process.env.ABSMARTLY_API_KEY, environment: process.env.NODE_ENV, application: process.env.APPLICATION_NAME, eventLogger: (context, eventName, data) => { - if (eventName == "error") { + if (eventName === "error") { console.error(data); } }, }); ``` -The data parameter depends on the type of event. -Currently, the SDK logs the following events: +**Event Types** -| eventName | when | data | -|:---: |---|---| -| `"error"` | `Context` receives an error | error object thrown | -| `"ready"` | `Context` turns ready | data used to initialize the context | -| `"refresh"` | `Context.refresh()` method succeeds | data used to refresh the context | -| `"publish"` | `Context.publish()` method succeeds | data sent to the A/B Smartly event collector | -| `"exposure"` | `Context.treatment()` method succeeds on first exposure | exposure data enqueued for publishing | -| `"goal"` | `Context.track()` method succeeds | goal data enqueued for publishing | -| `"finalize"` | `Context.finalize()` method succeeds the first time | undefined | +The data parameter depends on the type of event. Currently, the SDK logs the following events: +| Event | When | Data | +| :----------- | :------------------------------------------------ | :-------------------------------------------- | +| `"error"` | Context receives an error | Error object thrown | +| `"ready"` | Context turns ready | Data used to initialize the context | +| `"refresh"` | `refresh()` method succeeds | Data used to refresh the context | +| `"publish"` | `publish()` method succeeds | Data sent to the A/B Smartly event collector | +| `"exposure"` | `treatment()` method succeeds on first exposure | Exposure data enqueued for publishing | +| `"goal"` | `track()` method succeeds | Goal data enqueued for publishing | +| `"finalize"` | `finalize()` method succeeds the first time | undefined | -#### Peek at treatment variants -Although generally not recommended, it is sometimes necessary to peek at a treatment without triggering an exposure. -The A/B Smartly SDK provides a `peek()` method for that. +### HTTP Request Timeout -```javascript -if (context.peek("exp_test_experiment") == 0) { - // user is in control group (variant 0) -} else { - // user is in treatment group -} -``` - -#### Overriding treatment variants -During development, for example, it is useful to force a treatment for an experiment. This can be achieved with the `override()` and/or `overrides()` methods. -The `override()` and `overrides()` methods can be called before the context is ready. -```javascript - context.override("exp_test_experiment", 1); // force variant 1 of treatment - context.overrides({ - exp_test_experiment: 1, - exp_another_experiment: 0, - }); -``` - -#### HTTP request timeout -It is possible to set a timeout per individual HTTP request, overriding the global timeout set for all request when instantiating the SDK object. +It is possible to set a timeout per individual HTTP request, overriding the global timeout set for all requests when instantiating the SDK object. -Here is an example of setting a timeout only for the createContext request. +Here is an example of setting a timeout only for the `createContext` request. ```javascript const context = sdk.createContext(request, { refreshPeriod: 5 * 60 * 1000 }, { - timeout: 1500 + timeout: 1500, }); ``` -#### HTTP Request cancellation -Sometimes it is useful to cancel an inflight HTTP request, for example, when the user is navigating away. The A/B Smartly SDK also supports a cancellation via an `AbortSignal`. An implementation of AbortController is provided for older platforms, but will use the native implementation where available. +### HTTP Request Cancellation + +Sometimes it is useful to cancel an inflight HTTP request, for example, when the user is navigating away. The A/B Smartly SDK supports cancellation via an `AbortSignal`. An implementation of AbortController is provided for older platforms, but will use the native implementation where available. Here is an example of a cancellation scenario. @@ -309,7 +397,7 @@ const controller = new absmartly.AbortController(); const context = sdk.createContext(request, { refreshPeriod: 5 * 60 * 1000 }, { - signal: controller.signal + signal: controller.signal, }); // abort request if not ready after 1500ms @@ -320,14 +408,163 @@ await context.ready(); clearTimeout(timeoutId); ``` +## Node.js Usage + +### Express.js Middleware Example + +```javascript +const absmartly = require("@absmartly/javascript-sdk"); + +const sdk = new absmartly.SDK({ + endpoint: "https://your-company.absmartly.io/v1", + apiKey: process.env.ABSMARTLY_API_KEY, + environment: "production", + application: "website", +}); + +app.use(async (req, res, next) => { + const context = sdk.createContext({ + units: { + session_id: req.cookies.session_id, + }, + }); + + await context.ready(); + if (context.isFailed()) { + console.error("ABsmartly context failed:", context.readyError()); + } + req.absmartly = context; + next(); +}); + +app.get("/landing", (req, res) => { + const context = req.absmartly; + const treatment = context.treatment("exp_landing_page"); + + if (treatment === 0) { + res.render("landing-control"); + } else { + res.render("landing-treatment"); + } +}); +``` + +### Server-Side Rendering (SSR) with Data Forwarding + +Create the context on the server and pass the data to the client to avoid a second round-trip. + +```javascript +app.get("/", async (req, res) => { + const context = sdk.createContext({ + units: { session_id: req.cookies.session_id }, + }); + + await context.ready(); + + const contextData = context.data(); + const treatment = context.treatment("exp_homepage"); + + res.render("index", { + treatment, + absmartlyData: JSON.stringify(contextData), + }); +}); +``` + +On the client side, initialize the context with the pre-fetched data: + +```javascript +const context = sdk.createContextWith( + { units: { session_id: sessionId } }, + JSON.parse(window.__ABSMARTLY_DATA__) +); +// context is immediately ready, no round-trip needed +``` + +## Browser Usage + +### Single-Page Application (SPA) Example + +```javascript +import absmartly from "@absmartly/javascript-sdk"; + +const sdk = new absmartly.SDK({ + endpoint: "https://your-company.absmartly.io/v1", + apiKey: "YOUR_API_KEY", + environment: "production", + application: "website", + eventLogger: (context, eventName, data) => { + if (eventName === "exposure") { + analytics.track("Experiment Viewed", { + experiment: data.name, + variant: data.variant, + }); + } + }, +}); + +const context = sdk.createContext({ + units: { + session_id: getUserSessionId(), + }, +}); + +await context.ready(); + +const showNewFeature = context.treatment("exp_new_feature") !== 0; + +if (showNewFeature) { + renderNewFeature(); +} else { + renderOldFeature(); +} + +document.getElementById("checkout-btn").addEventListener("click", () => { + context.track("checkout", { total: getCartTotal() }); +}); +``` + +## Migration Guide + +This version includes minor breaking changes made for cross-SDK consistency and correctness. These align the JavaScript SDK with the Python, Swift, Java, and other A/B Smartly SDKs. + +### `ready()` no longer resolves with the Error object on failure + +**Before:** `ready()` resolved with the Error object on failure (e.g., `const result = await context.ready()` would give you the Error). + +**After:** `ready()` always resolves with `true`. It is a "wait for initialization" signal — you should always proceed with experiment code after it settles, even on failure. The SDK returns control variants (`0`) and default values gracefully when the API is down. Use `isFailed()` and `readyError()` to check for errors if needed. + +```javascript +await context.ready(); +if (context.isFailed()) { + console.error("Context failed:", context.readyError()); +} +const variant = context.treatment("exp_test"); // returns 0 (control) on failure +``` + +**When this might be a problem:** If your code used the return value as the Error object (e.g., `const err = await context.ready(); logError(err)`), it will now receive `true` instead. Use `context.readyError()` to access the error instead. + +### `override()` now throws after finalization + +`override()` now calls `_checkNotFinalized()`, consistent with `customAssignment()`, `track()`, and `attribute()`. Previously, overrides could be set on a finalized context silently with no effect. ## About A/B Smartly -**A/B Smartly** is the leading provider of state-of-the-art, on-premises, full-stack experimentation platforms for engineering and product teams that want to confidently deploy features as fast as they can develop them. -A/B Smartly's real-time analytics helps engineering and product teams ensure that new features will improve the customer experience without breaking or degrading performance and/or business metrics. + +**A/B Smartly** is the leading provider of state-of-the-art, on-premises, full-stack experimentation platforms for engineering and product teams that want to confidently deploy features as fast as they can develop them. A/B Smartly's real-time analytics helps engineering and product teams ensure that new features will improve the customer experience without breaking or degrading performance and/or business metrics. ### Have a look at our growing list of clients and SDKs: +- [JavaScript SDK](https://www.github.com/absmartly/javascript-sdk) (this package) +- [React SDK](https://www.github.com/absmartly/react-sdk) +- [Vue2 SDK](https://www.github.com/absmartly/vue2-sdk) +- [Vue3 SDK](https://www.github.com/absmartly/vue3-sdk) - [Java SDK](https://www.github.com/absmartly/java-sdk) -- [JavaScript SDK](https://www.github.com/absmartly/javascript-sdk) -- [PHP SDK](https://www.github.com/absmartly/php-sdk) +- [Android SDK](https://www.github.com/absmartly/android-sdk) - [Swift SDK](https://www.github.com/absmartly/swift-sdk) -- [Vue2 SDK](https://www.github.com/absmartly/vue2-sdk) +- [Dart SDK](https://www.github.com/absmartly/dart-sdk) +- [Flutter SDK](https://www.github.com/absmartly/flutter-sdk) +- [PHP SDK](https://www.github.com/absmartly/php-sdk) +- [Python3 SDK](https://www.github.com/absmartly/python3-sdk) +- [Go SDK](https://www.github.com/absmartly/go-sdk) +- [Ruby SDK](https://www.github.com/absmartly/ruby-sdk) +- [.NET SDK](https://www.github.com/absmartly/dotnet-sdk) +- [Rust SDK](https://www.github.com/absmartly/rust-sdk) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index fc080e5..6d0a34e 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -166,7 +166,7 @@ describe("Context", () => { config: '{"card.width":"75%"}', }, ], - audience: "{}", + audience: "", customFieldValues: null, }, { @@ -204,7 +204,7 @@ describe("Context", () => { config: '{"submit.color":"green","submit.shape":"square"}', }, ], - audience: "null", + audience: "", customFieldValues: null, }, { @@ -612,12 +612,12 @@ describe("Context", () => { expect(context.isFinalized()).toEqual(false); expect(() => context.data()).toThrow(); - expect(() => context.treatment("test")).toThrow(); - expect(() => context.peek("test")).toThrow(); - expect(() => context.experiments()).toThrow(); - expect(() => context.variableKeys()).toThrow(); - expect(() => context.variableValue("a", 17)).toThrow(); - expect(() => context.peekVariableValue("a", 17)).toThrow(); + expect(() => context.treatment("test")).toThrow("ABsmartly Context is not yet ready."); + expect(() => context.peek("test")).toThrow("ABsmartly Context is not yet ready."); + expect(() => context.experiments()).toThrow("ABsmartly Context is not yet ready."); + expect(() => context.variableKeys()).toThrow("ABsmartly Context is not yet ready."); + expect(() => context.variableValue("a", 17)).toThrow("ABsmartly Context is not yet ready."); + expect(() => context.peekVariableValue("a", 17)).toThrow("ABsmartly Context is not yet ready."); done(); }); @@ -1074,6 +1074,154 @@ describe("Context", () => { }); }); + it("should clear assignment cache for started experiment", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + expect(context.treatment("exp_test_new")).toEqual(0); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(2); + + provider.getContextData.mockReturnValue(Promise.resolve(refreshContextResponse)); + + context.refresh().then(() => { + expect(context.treatment("exp_test_new")).toEqual(expectedVariants["exp_test_new"]); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(3); + + done(); + }); + }); + + it("should clear assignment cache for stopped experiment", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(2); + + const refreshWithStoppedExperiment = { + ...getContextResponse, + experiments: getContextResponse.experiments.filter((x) => x.name !== "exp_test_abc"), + }; + + provider.getContextData.mockReturnValue(Promise.resolve(refreshWithStoppedExperiment)); + + context.refresh().then(() => { + expect(context.treatment("exp_test_abc")).toEqual(0); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(3); + + done(); + }); + }); + + it("should clear assignment cache when experiment ID changes", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(2); + + const refreshWithChangedId = { + ...getContextResponse, + experiments: getContextResponse.experiments.map((x) => { + if (x.name === "exp_test_abc") { + return { + ...x, + id: 11, + trafficSeedHi: 54870830, + trafficSeedLo: 398724581, + seedHi: 77498863, + seedLo: 34737352, + }; + } + return x; + }), + }; + + provider.getContextData.mockReturnValue(Promise.resolve(refreshWithChangedId)); + + context.refresh().then(() => { + expect(context.treatment("exp_test_abc")).toEqual(2); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(3); + + done(); + }); + }); + + it("should clear assignment cache when full-on changes", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + expect(context.treatment("exp_test_abc")).toEqual(expectedVariants["exp_test_abc"]); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(2); + + const refreshWithFullOn = { + ...getContextResponse, + experiments: getContextResponse.experiments.map((x) => { + if (x.name === "exp_test_abc") { + return { + ...x, + fullOnVariant: 1, + }; + } + return x; + }), + }; + + provider.getContextData.mockReturnValue(Promise.resolve(refreshWithFullOn)); + + context.refresh().then(() => { + expect(context.treatment("exp_test_abc")).toEqual(1); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(3); + + done(); + }); + }); + + it("should clear assignment cache when traffic split changes", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + expect(context.treatment("exp_test_not_eligible")).toEqual(expectedVariants["exp_test_not_eligible"]); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(2); + + const refreshWithTrafficSplit = { + ...getContextResponse, + experiments: getContextResponse.experiments.map((x) => { + if (x.name === "exp_test_not_eligible") { + return { + ...x, + trafficSplit: [0.0, 1.0], + }; + } + return x; + }), + }; + + provider.getContextData.mockReturnValue(Promise.resolve(refreshWithTrafficSplit)); + + context.refresh().then(() => { + expect(context.treatment("exp_test_not_eligible")).toEqual(2); + expect(context.treatment("not_found")).toEqual(0); + + expect(context.pending()).toEqual(3); + + done(); + }); + }); + it("should throw after finalized() call", (done) => { const context = new Context(sdk, contextOptions, contextParams, getContextResponse); publisher.publish.mockReturnValue(Promise.resolve()); @@ -1372,6 +1520,30 @@ describe("Context", () => { done(); }); + + it("should throw when not ready", (done) => { + const context = new Context(sdk, contextOptions, contextParams, Promise.resolve(getContextResponse)); + expect(context.isReady()).toEqual(false); + + expect(() => context.peek("exp_test_ab")).toThrow("ABsmartly Context is not yet ready."); + + done(); + }); + + it("should throw after finalize", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + publisher.publish.mockReturnValue(Promise.resolve()); + + context.treatment("exp_test_ab"); + + context.finalize().then(() => { + expect(() => context.peek("exp_test_ab")).toThrow("ABsmartly Context is finalized."); + done(); + }); + + expect(context.isFinalizing()).toEqual(true); + expect(() => context.peek("exp_test_ab")).toThrow("ABsmartly Context is finalizing."); + }); }); describe("treatment()", () => { @@ -1815,13 +1987,13 @@ describe("Context", () => { expect(context.pending()).toEqual(1); context.finalize().then(() => { - expect(() => context.treatment("exp_test_ab")).toThrow(); + expect(() => context.treatment("exp_test_ab")).toThrow("ABsmartly Context is finalized."); done(); }); expect(context.isFinalizing()).toEqual(true); - expect(() => context.treatment("exp_test_ab")).toThrow(); + expect(() => context.treatment("exp_test_ab")).toThrow("ABsmartly Context is finalizing."); }); it("should re-evaluate audience expression when attributes change in strict mode", (done) => { @@ -2021,6 +2193,15 @@ describe("Context", () => { done(); }); + it("should throw when not ready", (done) => { + const context = new Context(sdk, contextOptions, contextParams, Promise.resolve(getContextResponse)); + expect(context.isReady()).toEqual(false); + + expect(() => context.treatment("exp_test_ab")).toThrow("ABsmartly Context is not yet ready."); + + done(); + }); + it("should update attrsSeq after checking unchanged audience to avoid repeated evaluation", (done) => { const context = new Context(sdk, contextOptions, contextParams, audienceStrictContextResponse); @@ -3128,13 +3309,13 @@ describe("Context", () => { expect(context.pending()).toEqual(1); context.finalize().then(() => { - expect(() => context.variableValue("button.color", 17)).toThrow(); + expect(() => context.variableValue("button.color", 17)).toThrow("ABsmartly Context is finalized."); done(); }); expect(context.isFinalizing()).toEqual(true); - expect(() => context.variableValue("button.color", 17)).toThrow(); + expect(() => context.variableValue("button.color", 17)).toThrow("ABsmartly Context is finalizing."); }); }); @@ -4330,6 +4511,84 @@ describe("Context", () => { }); }); }); + + it("should clear assignment cache when override changes", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + context.override("exp_test_ab", 2); + context.treatment("exp_test_ab"); + + expect(context.pending()).toEqual(1); + + context.override("exp_test_ab", 2); + context.treatment("exp_test_ab"); + + expect(context.pending()).toEqual(1); + + context.override("exp_test_ab", 3); + context.treatment("exp_test_ab"); + + expect(context.pending()).toEqual(2); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish).toHaveBeenCalledWith( + { + publishedAt: 1611141535729, + units: publishUnits, + hashed: true, + exposures: [ + { + id: 1, + name: "exp_test_ab", + unit: "session_id", + exposedAt: 1611141535729, + variant: 2, + assigned: false, + eligible: true, + overridden: true, + fullOn: false, + custom: false, + audienceMismatch: false, + }, + { + id: 1, + name: "exp_test_ab", + unit: "session_id", + exposedAt: 1611141535729, + variant: 3, + assigned: false, + eligible: true, + overridden: true, + fullOn: false, + custom: false, + audienceMismatch: false, + }, + ], + }, + sdk, + context, + undefined + ); + + done(); + }); + }); + + it("should clear assignment cache when overriding computed assignment", (done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + expect(context.treatment("exp_test_ab")).toEqual(expectedVariants["exp_test_ab"]); + expect(context.pending()).toEqual(1); + + context.override("exp_test_ab", 9); + expect(context.treatment("exp_test_ab")).toEqual(9); + + expect(context.pending()).toEqual(2); + + done(); + }); }); describe("customAssignment()", () => { @@ -4606,27 +4865,28 @@ describe("Context", () => { expect(context.customFieldValue("exp_test_custom_fields", "false_boolean_field")).toEqual(false); }); - it("should console an error when JSON cannot be parsed", () => { - const errorSpy = jest.spyOn(console, "error"); - const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + it("should log an error through eventLogger when JSON cannot be parsed", () => { + const eventLogger = jest.fn(); + const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, getContextResponse); expect(context.pending()).toEqual(0); expect(context.customFieldValue("exp_test_abc", "json_invalid")).toEqual(null); - expect(errorSpy).toHaveBeenCalledTimes(1); - expect(errorSpy).toHaveBeenCalledWith( - "Failed to parse JSON custom field value 'json_invalid' for experiment 'exp_test_abc'" - ); + expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); }); - it("should console an error when a field type is invalid", () => { - const errorSpy = jest.spyOn(console, "error"); - const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + it("should log an error through eventLogger when a field type is invalid", () => { + const eventLogger = jest.fn(); + const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, getContextResponse); expect(context.pending()).toEqual(0); expect(context.customFieldValue("exp_test_custom_fields", "invalid_type_field")).toEqual(null); - expect(errorSpy).toHaveBeenCalledTimes(1); - expect(errorSpy).toHaveBeenCalledWith( - "Unknown custom field type 'invalid' for experiment 'exp_test_custom_fields' and key 'invalid_type_field' - you may need to upgrade to the latest SDK version" + expect(eventLogger).toHaveBeenCalledWith( + context, + "error", + expect.objectContaining({ + message: + "Unknown custom field type 'invalid' for experiment 'exp_test_custom_fields' and key 'invalid_type_field' - you may need to upgrade to the latest SDK version", + }) ); }); }); diff --git a/src/__tests__/fixes.test.js b/src/__tests__/fixes.test.js new file mode 100644 index 0000000..1cf08d8 --- /dev/null +++ b/src/__tests__/fixes.test.js @@ -0,0 +1,475 @@ +import { MatchOperator } from "../jsonexpr/operators/match"; +import { EqualsOperator } from "../jsonexpr/operators/eq"; +import { mockEvaluator } from "./jsonexpr/operators/evaluator"; +import { AbortController as ShimAbortController, AbortSignal as ShimAbortSignal } from "../abort-controller-shim"; +import { AudienceMatcher } from "../matcher"; +import SDK from "../sdk"; +import Context from "../context"; +import Client from "../client"; +import { ContextPublisher } from "../publisher"; +import { ContextDataProvider } from "../provider"; + +jest.mock("../client"); +jest.mock("../sdk"); +jest.mock("../provider"); +jest.mock("../publisher"); + +describe("Fix #1: ReDoS protection in MatchOperator", () => { + const operator = new MatchOperator(); + const evaluator = mockEvaluator(); + + it("should reject patterns with nested quantifiers", () => { + expect(operator.evaluate(evaluator, ["aaaaaaaaaa", "(a+)+$"])).toBe(null); + expect(operator.evaluate(evaluator, ["test", "(a*)*b"])).toBe(null); + expect(operator.evaluate(evaluator, ["test", "(x+){2}"])).toBe(null); + }); + + it("should still allow safe patterns", () => { + expect(operator.evaluate(evaluator, ["abc", "a+"])).toBe(true); + expect(operator.evaluate(evaluator, ["abc", "^abc$"])).toBe(true); + expect(operator.evaluate(evaluator, ["abc", "[a-z]+"])).toBe(true); + }); + + it("should reject patterns exceeding max length", () => { + const longPattern = "a".repeat(1001); + expect(operator.evaluate(evaluator, ["test", longPattern])).toBe(null); + }); + + it("should reject text exceeding max length", () => { + const longText = "a".repeat(10001); + expect(operator.evaluate(evaluator, [longText, "a"])).toBe(null); + }); + + it("should cache compiled regexes", () => { + expect(operator.evaluate(evaluator, ["abc", "abc"])).toBe(true); + expect(operator.evaluate(evaluator, ["abc", "abc"])).toBe(true); + }); + + it("should return null for invalid regex", () => { + expect(operator.evaluate(evaluator, ["test", "[invalid"])).toBe(null); + }); + + it("should not use console.error", () => { + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + operator.evaluate(evaluator, ["test", "[invalid"]); + operator.evaluate(evaluator, ["test", "a".repeat(1001)]); + operator.evaluate(evaluator, ["a".repeat(10001), "a"]); + expect(errorSpy).not.toHaveBeenCalled(); + errorSpy.mockRestore(); + }); +}); + +describe("Fix #2: ready() error handling and readyError()", () => { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + sdk.getEventLogger.mockReturnValue(SDK.defaultEventLogger); + + const contextOptions = { + publishDelay: -1, + refreshPeriod: 0, + }; + + const contextParams = { + units: { + session_id: "test-session", + }, + }; + + it("should store error via readyError() when context fetch fails", async () => { + const error = new Error("fetch failed"); + const context = new Context(sdk, contextOptions, contextParams, Promise.reject(error)); + await context.ready(); + + expect(context.isFailed()).toBe(true); + expect(context.readyError()).toBe(error); + }); + + it("should return null for readyError() when no failure", () => { + const context = new Context(sdk, contextOptions, contextParams, { experiments: [] }); + expect(context.readyError()).toBe(null); + }); + + it("should allow treatment/peek/track calls after failed init without throwing", async () => { + const error = new Error("fetch failed"); + const context = new Context(sdk, contextOptions, contextParams, Promise.reject(error)); + const result = await context.ready(); + + expect(result).toBe(true); + expect(context.isFailed()).toBe(true); + expect(context.isReady()).toBe(true); + + expect(context.treatment("any_experiment")).toBe(0); + expect(context.peek("any_experiment")).toBe(0); + expect(context.variableValue("any_key", "fallback")).toBe("fallback"); + expect(context.peekVariableValue("any_key", "fallback")).toBe("fallback"); + expect(context.experiments()).toEqual([]); + expect(context.variableKeys()).toEqual({}); + + expect(() => context.track("goal_name")).not.toThrow(); + expect(() => context.attribute("attr", "value")).not.toThrow(); + }); +}); + +describe("Fix #3: for...of on array in _resolveVariableValue", () => { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + sdk.getEventLogger.mockReturnValue(SDK.defaultEventLogger); + + const contextOptions = { + publishDelay: -1, + refreshPeriod: 0, + }; + + const contextParams = { + units: { + session_id: "e791e240fcd3df7d238cfc285f475e8152fcc0ec", + }, + }; + + it("should handle unknown variable keys without error", () => { + const context = new Context(sdk, contextOptions, contextParams, { + experiments: [ + { + id: 1, + name: "exp_test", + iteration: 1, + unitType: "session_id", + seedHi: 3603515, + seedLo: 233373850, + split: [0.5, 0.5], + trafficSeedHi: 449867249, + trafficSeedLo: 455443629, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + audience: null, + audienceStrict: false, + variants: [ + { name: "A", config: null }, + { name: "B", config: '{"color":"red"}' }, + ], + customFieldValues: null, + }, + ], + }); + + expect(context.variableValue("nonexistent_key", "default")).toBe("default"); + }); +}); + +describe("Fix #4: console.error routed through eventLogger", () => { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + + const contextParams = { + units: { + session_id: "test", + }, + }; + + it("should not call console.error directly for custom field parse errors", () => { + const eventLogger = jest.fn(); + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + sdk.getEventLogger.mockReturnValue(eventLogger); + const context = new Context(sdk, { publishDelay: -1, refreshPeriod: 0, eventLogger }, contextParams, { + experiments: [ + { + id: 1, + name: "exp", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 1, + split: [1], + trafficSeedHi: 1, + trafficSeedLo: 1, + trafficSplit: [0, 1], + fullOnVariant: 0, + audience: null, + audienceStrict: false, + variants: [{ name: "A", config: null }], + customFieldValues: [{ name: "bad_json", value: "{invalid", type: "json" }], + }, + ], + }); + + context.customFieldValue("exp", "bad_json"); + expect(errorSpy).not.toHaveBeenCalled(); + expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); + errorSpy.mockRestore(); + }); + + it("should not call console.error in AudienceMatcher on parse failure", () => { + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + const matcher = new AudienceMatcher(); + matcher.evaluate("{invalid json", {}); + expect(errorSpy).not.toHaveBeenCalled(); + errorSpy.mockRestore(); + }); + + it("should route variant config parse errors through eventLogger", () => { + const eventLogger = jest.fn(); + sdk.getEventLogger.mockReturnValue(eventLogger); + + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + const context = new Context(sdk, { publishDelay: -1, refreshPeriod: 0, eventLogger }, contextParams, { + experiments: [ + { + id: 1, + name: "exp_bad_config", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 1, + split: [1], + trafficSeedHi: 1, + trafficSeedLo: 1, + trafficSplit: [0, 1], + fullOnVariant: 0, + audience: null, + audienceStrict: false, + variants: [{ name: "A", config: "{invalid json}" }], + customFieldValues: null, + }, + ], + }); + + expect(errorSpy).not.toHaveBeenCalled(); + expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); + errorSpy.mockRestore(); + }); +}); + +describe("Fix #5: _finalizing type cleanup", () => { + it("should not use boolean for _finalizing", () => { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + sdk.getEventLogger.mockReturnValue(jest.fn()); + + const context = new Context( + sdk, + { publishDelay: -1, refreshPeriod: 0 }, + { units: { session_id: "test" } }, + { experiments: [] } + ); + + expect(context.isFinalizing()).toBe(false); + expect(context.isFinalized()).toBe(false); + }); +}); + +describe("Fix #11: SDK._extractClientOptions uses includes", () => { + it("should extract client options correctly", () => { + const sdk = new SDK({ + agent: "test-agent", + apiKey: "key", + application: "app", + endpoint: "http://localhost", + environment: "test", + timeout: 5000, + }); + expect(sdk).toBeInstanceOf(SDK); + }); +}); + +describe("Fix #12: SDK.defaultEventLogger logs error message", () => { + let ActualSDK; + beforeAll(() => { + ActualSDK = jest.requireActual("../sdk").default; + }); + + it("should log full Error object to preserve stack traces", () => { + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + const error = new Error("something failed"); + ActualSDK.defaultEventLogger(null, "error", error); + expect(errorSpy).toHaveBeenCalledWith(error); + errorSpy.mockRestore(); + }); + + it("should log raw data for non-Error values", () => { + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + ActualSDK.defaultEventLogger(null, "error", "plain text error"); + expect(errorSpy).toHaveBeenCalledWith("plain text error"); + errorSpy.mockRestore(); + }); +}); + +describe("Fix #13: _getAttributesMap caching", () => { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + sdk.getEventLogger.mockReturnValue(jest.fn()); + + it("should return correct attributes after multiple attribute() calls", () => { + const context = new Context( + sdk, + { publishDelay: -1, refreshPeriod: 0 }, + { units: { session_id: "test" } }, + { experiments: [] } + ); + + context.attribute("age", 25); + context.attribute("country", "US"); + + const attrs = context.getAttributes(); + expect(attrs).toEqual({ age: 25, country: "US" }); + }); +}); + +describe("Fix #15: fetch.ts throws on missing implementation", () => { + it("should not export undefined", async () => { + const fetchModule = await import("../fetch"); + const fetchImpl = fetchModule.default; + expect(fetchImpl).not.toBeUndefined(); + expect(typeof fetchImpl).toBe("function"); + }); +}); + +describe("Fix #16: Client.request timeout uses nullish coalescing", () => { + it("should accept timeout of 0 in options", () => { + const client = new Client({ + endpoint: "http://test", + agent: "test", + environment: "test", + apiKey: "key", + application: "app", + timeout: 5000, + }); + + expect(client).toBeInstanceOf(Client); + }); +}); + +describe("Fix #21: AbortController shim sets signal.reason", () => { + it("should set default reason on abort()", () => { + const controller = new ShimAbortController(); + controller.abort(); + expect(controller.signal.aborted).toBe(true); + expect(controller.signal.reason).toBeInstanceOf(Error); + expect(controller.signal.reason.message).toBe("The operation was aborted."); + }); + + it("should set custom reason on abort(reason)", () => { + const controller = new ShimAbortController(); + const customReason = new Error("custom abort"); + controller.abort(customReason); + expect(controller.signal.reason).toBe(customReason); + }); + + it("should have undefined reason before abort", () => { + const controller = new ShimAbortController(); + expect(controller.signal.reason).toBeUndefined(); + }); +}); + +describe("Fix #25: Context.getOptions() returns shallow copy", () => { + it("should not allow mutation of internal options", () => { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + sdk.getEventLogger.mockReturnValue(jest.fn()); + + const originalOptions = { publishDelay: 100, refreshPeriod: 0 }; + const context = new Context(sdk, originalOptions, { units: { session_id: "test" } }, { experiments: [] }); + + const opts = context.getOptions(); + opts.publishDelay = 9999; + + expect(context.getOptions().publishDelay).toBe(100); + }); +}); + +describe("Fix #32: AbortSignal dispatchEvent uses explicit onabort", () => { + it("should call onabort handler on dispatch", () => { + const signal = new ShimAbortSignal(); + const handler = jest.fn(); + signal.onabort = handler; + signal.dispatchEvent({ type: "abort" }); + expect(handler).toHaveBeenCalledTimes(1); + expect(handler).toHaveBeenCalledWith({ type: "abort" }); + }); + + it("should not call onabort for non-abort events", () => { + const signal = new ShimAbortSignal(); + const handler = jest.fn(); + signal.onabort = handler; + signal.dispatchEvent({ type: "other" }); + expect(handler).not.toHaveBeenCalled(); + }); +}); + +describe("Fix #33: Client constructor uses spread", () => { + it("should merge defaults with provided options", () => { + const client = new Client({ + endpoint: "http://test", + agent: "custom-agent", + environment: "prod", + apiKey: "key123", + application: "myapp", + timeout: 10000, + }); + + expect(client).toBeInstanceOf(Client); + }); +}); + +describe("Fix #34: SDK._contextOptions uses spread", () => { + it("should merge custom options with defaults", () => { + const sdk = new SDK({ + agent: "test", + apiKey: "key", + application: "app", + endpoint: "http://localhost", + environment: "test", + }); + + expect(sdk).toBeInstanceOf(SDK); + }); +}); + +describe("Fix #20: EqualsOperator without redundant Array.isArray", () => { + const operator = new EqualsOperator(); + const evaluator = mockEvaluator(); + + it("should evaluate equality correctly", () => { + expect(operator.evaluate(evaluator, [1, 1])).toBe(true); + expect(operator.evaluate(evaluator, [1, 2])).toBe(false); + }); + + it("should handle null comparison", () => { + expect(operator.evaluate(evaluator, [null, null])).toBe(null); + }); + + it("should handle empty args", () => { + expect(operator.evaluate(evaluator, [])).toBe(null); + }); +}); diff --git a/src/__tests__/jsonexpr/operators/eq.test.js b/src/__tests__/jsonexpr/operators/eq.test.js index d7132af..82d507d 100644 --- a/src/__tests__/jsonexpr/operators/eq.test.js +++ b/src/__tests__/jsonexpr/operators/eq.test.js @@ -39,9 +39,11 @@ describe("EqOperator", () => { evaluator.compare.mockClear(); expect(operator.evaluate(evaluator, [null, null])).toBe(null); - expect(evaluator.evaluate).toHaveBeenCalledTimes(1); + expect(evaluator.evaluate).toHaveBeenCalledTimes(2); expect(evaluator.evaluate).toHaveBeenNthCalledWith(1, null); - expect(evaluator.compare).toHaveBeenCalledTimes(0); + expect(evaluator.evaluate).toHaveBeenNthCalledWith(2, null); + expect(evaluator.compare).toHaveBeenCalledTimes(1); + expect(evaluator.compare).toHaveBeenCalledWith(null, null); evaluator.evaluate.mockClear(); evaluator.compare.mockClear(); diff --git a/src/abort-controller-shim.ts b/src/abort-controller-shim.ts index 1456e47..af18593 100644 --- a/src/abort-controller-shim.ts +++ b/src/abort-controller-shim.ts @@ -5,6 +5,8 @@ export type AbortControllerEvents = { // eslint-disable-next-line no-shadow export class AbortSignal { aborted = false; + reason: unknown = undefined; + onabort?: ((evt: { type: string }) => void) | null; private readonly _events: AbortControllerEvents; constructor() { @@ -34,9 +36,9 @@ export class AbortSignal { } dispatchEvent(evt: { type: string }) { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - this[`on${evt.type}`] && this[`on${evt.type}`](evt); + if (evt.type === "abort" && this.onabort) { + this.onabort(evt); + } const listeners = this._events[evt.type]; if (listeners) { for (const listener of listeners) { @@ -54,11 +56,11 @@ export class AbortSignal { export class AbortController { signal = new AbortSignal(); - abort() { + abort(reason?: unknown) { let evt: Event | { type: string; bubbles: boolean; cancelable: boolean }; try { evt = new Event("abort"); - } catch (e) { + } catch (error) { evt = { type: "abort", bubbles: false, @@ -67,6 +69,7 @@ export class AbortController { } this.signal.aborted = true; + this.signal.reason = reason ?? new Error("The operation was aborted."); this.signal.dispatchEvent(evt); } diff --git a/src/client.ts b/src/client.ts index 56d91a2..95ede25 100644 --- a/src/client.ts +++ b/src/client.ts @@ -4,9 +4,9 @@ import { AbortController } from "./abort"; // eslint-disable-next-line no-shadow import { AbortError, RetryError, TimeoutError } from "./errors"; -import { AbortSignal as ABsmartlyAbortSignal } from "./abort-controller-shim"; -import { ContextOptions, ContextParams } from "./context"; -import { PublishParams } from "./publisher"; +import { type AbortSignal as ABsmartlyAbortSignal } from "./abort-controller-shim"; +import { type ContextOptions, type ContextParams } from "./context"; +import { type PublishParams } from "./publisher"; export type FetchResponse = { status: number; @@ -29,6 +29,10 @@ export type ClientRequestOptions = { export type ApplicationObject = { name: string; version: number | string }; +const DEFAULT_RETRIES = 5; +const DEFAULT_TIMEOUT_MS = 3000; +const RETRY_DELAY_MS = 50; + export type ClientOptions = { agent?: string; apiKey: string; @@ -52,8 +56,8 @@ export default class Client { const merged: Record = Object.assign( { agent: "javascript-client", - retries: 5, - timeout: 3000, + retries: DEFAULT_RETRIES, + timeout: DEFAULT_TIMEOUT_MS, keepalive: true, }, opts @@ -83,7 +87,7 @@ export default class Client { } this._opts = merged as unknown as NormalizedClientOptions; - this._delay = 50; + this._delay = RETRY_DELAY_MS; } getEnvironment(): string { @@ -143,12 +147,13 @@ export default class Client { request(options: ClientRequestOptions) { let url = `${this._opts.endpoint}${options.path}`; if (options.query) { - const keys = Object.keys(options.query); - if (keys.length > 0) { - const encoded = keys - .map((k) => (options.query ? `${k}=${encodeURIComponent(options.query[k])}` : null)) - .join("&"); - url = `${url}?${encoded}`; + const params = new URLSearchParams(); + for (const [key, value] of Object.entries(options.query)) { + params.append(key, String(value)); + } + const queryString = params.toString(); + if (queryString) { + url = `${url}?${queryString}`; } } @@ -258,7 +263,7 @@ export default class Client { options.signal.addEventListener("abort", abort); } - const timeout = options.timeout || this._opts.timeout || 0; + const timeout = options.timeout ?? this._opts.timeout ?? 0; const timeoutId = timeout > 0 ? setTimeout(() => { @@ -274,7 +279,7 @@ export default class Client { } }; - return tryWith(this._opts.retries ?? 5, this._opts.timeout ?? 3000) + return tryWith(this._opts.retries ?? DEFAULT_RETRIES, timeout) .then((value: string) => { finalCleanUp(); return value; diff --git a/src/context.ts b/src/context.ts index 673f80f..73304b4 100644 --- a/src/context.ts +++ b/src/context.ts @@ -2,10 +2,10 @@ import { arrayEqualsShallow, hashUnit, isObject, isPromise } from "./utils"; import { VariantAssigner } from "./assigner"; import { AudienceMatcher } from "./matcher"; import { insertUniqueSorted } from "./algorithm"; -import SDK, { EventLogger, EventName } from "./sdk"; -import { ContextPublisher, PublishParams } from "./publisher"; +import SDK, { type EventLogger, type EventName } from "./sdk"; +import { ContextPublisher, type PublishParams } from "./publisher"; import { ContextDataProvider } from "./provider"; -import { ClientRequestOptions } from "./client"; +import { type ClientRequestOptions } from "./client"; import { SDK_VERSION } from "./version"; type JSONPrimitive = string | number | boolean | null; @@ -144,14 +144,17 @@ export default class Context { private _data: ContextData; private _exposures: Exposure[]; private _failed: boolean; + private _failedError: Error | null; private _finalized: boolean; - private _finalizing: boolean | Promise | null; + private _finalizing: Promise | null; private _goals: Goal[]; private _index: Record; private _indexVariables: Record; private _overrides: Record; private _pending: number; private _attrsSeq: number; + private _attrsMapCache: Record | null; + private _attrsMapCacheSeq: number; private _hashes?: Record; private _promise?: Promise; private _publishTimeout?: ReturnType; @@ -165,6 +168,7 @@ export default class Context { this._opts = options; this._pending = 0; this._failed = false; + this._failedError = null; this._finalized = false; this._attrs = []; this._goals = []; @@ -176,6 +180,8 @@ export default class Context { this._audienceMatcher = new AudienceMatcher(); this._environmentName = null; this._attrsSeq = 0; + this._attrsMapCache = null; + this._attrsMapCacheSeq = -1; if (params.units) { this.units(params.units); @@ -197,6 +203,7 @@ export default class Context { this._init({}); this._failed = true; + this._failedError = error; delete this._promise; this._logError(error); @@ -225,14 +232,16 @@ export default class Context { return this._failed; } + readyError(): Error | null { + return this._failedError; + } + ready() { if (this.isReady()) { return Promise.resolve(true); } - return new Promise((resolve) => { - this._promise?.then(() => resolve(true)).catch((e) => resolve(e)); - }); + return this._promise?.then(() => true) ?? Promise.resolve(true); } pending() { @@ -312,26 +321,27 @@ export default class Context { } getUnits() { - return this._units; + return { ...this._units }; } units(units: Record) { - Object.entries(units).forEach(([unitType, uid]) => { + for (const [unitType, uid] of Object.entries(units)) { this.unit(unitType, uid); - }); + } } getAttribute(attrName: string) { let result; - - this._attrs.forEach((attr) => { + for (const attr of this._attrs) { if (attr.name === attrName) result = attr.value; - }); - + } return result; } attribute(attrName: string, value: unknown) { + if (typeof attrName !== "string" || attrName.trim().length === 0) { + throw new Error("Attribute name must be a non-empty string"); + } this._checkNotFinalized(); this._attrs.push({ name: attrName, value: value, setAt: Date.now() }); @@ -340,36 +350,43 @@ export default class Context { getAttributes() { const attributes: Record = {}; - this._attrs - .map((a) => [a.name, a.value]) - .forEach(([key, value]) => { - attributes[key as string] = value; - }); + for (const attr of this._attrs) { + attributes[attr.name] = attr.value; + } return attributes; } attributes(attrs: Record) { - Object.entries(attrs).forEach(([attrName, value]) => { + for (const [attrName, value] of Object.entries(attrs)) { this.attribute(attrName, value); - }); + } } peek(experimentName: string) { + if (typeof experimentName !== "string" || experimentName.trim().length === 0) { + throw new Error("Experiment name must be a non-empty string"); + } this._checkReady(true); return this._peek(experimentName).variant; } treatment(experimentName: string) { + if (typeof experimentName !== "string" || experimentName.trim().length === 0) { + throw new Error("Experiment name must be a non-empty string"); + } this._checkReady(true); return this._treatment(experimentName).variant; } - track(goalName: string, properties?: Record) { + track(goalName: string, properties?: Record | null) { + if (typeof goalName !== "string" || goalName.trim().length === 0) { + throw new Error("Goal name must be a non-empty string"); + } this._checkNotFinalized(); - return this._track(goalName, properties); + return this._track(goalName, properties ?? undefined); } finalize(requestOptions?: ClientRequestOptions) { @@ -379,16 +396,22 @@ export default class Context { experiments() { this._checkReady(); - return this._data.experiments?.map((x) => x.name); + return this._data.experiments?.map((x) => x.name) ?? []; } variableValue(key: string, defaultValue: string): string { + if (typeof key !== "string" || key.trim().length === 0) { + throw new Error("Variable key must be a non-empty string"); + } this._checkReady(true); return this._variableValue(key, defaultValue); } peekVariableValue(key: string, defaultValue: string): string { + if (typeof key !== "string" || key.trim().length === 0) { + throw new Error("Variable key must be a non-empty string"); + } this._checkReady(true); return this._peekVariable(key, defaultValue); @@ -399,43 +422,64 @@ export default class Context { const variableExperiments: Record = {}; - Object.entries(this._indexVariables).forEach(([key, values]) => { - values.forEach((value) => { + for (const [key, values] of Object.entries(this._indexVariables)) { + for (const value of values) { if (variableExperiments[key]) variableExperiments[key].push(value.data.name); else variableExperiments[key] = [value.data.name]; - }); - }); + } + } return variableExperiments; } override(experimentName: string, variant: number) { + if (typeof experimentName !== "string" || experimentName.trim().length === 0) { + throw new Error("Experiment name must be a non-empty string"); + } + if (typeof variant !== "number" || variant < 0 || !Number.isInteger(variant)) { + throw new Error("Variant must be a non-negative integer"); + } + this._checkNotFinalized(); this._overrides = Object.assign(this._overrides, { [experimentName]: variant }); } overrides(experimentVariants: Record) { - Object.entries(experimentVariants).forEach(([experimentName, variant]) => { + for (const [experimentName, variant] of Object.entries(experimentVariants)) { this.override(experimentName, variant); - }); + } } customAssignment(experimentName: string, variant: number) { + if (typeof experimentName !== "string" || experimentName.trim().length === 0) { + throw new Error("Experiment name must be a non-empty string"); + } + if (typeof variant !== "number" || variant < 0 || !Number.isInteger(variant)) { + throw new Error("Variant must be a non-negative integer"); + } this._checkNotFinalized(); this._cassignments[experimentName] = variant; } customAssignments(experimentVariants: Record) { - Object.entries(experimentVariants).forEach(([experimentName, variant]) => { + for (const [experimentName, variant] of Object.entries(experimentVariants)) { this.customAssignment(experimentName, variant); - }); + } + } + + getSDK(): SDK { + return this._sdk; + } + + getOptions(): ContextOptions { + return { ...this._opts }; } private _checkNotFinalized() { if (this.isFinalized()) { - throw new Error("ABSmartly Context is finalized."); + throw new Error("ABsmartly Context is finalized."); } else if (this.isFinalizing()) { - throw new Error("ABSmartly Context is finalizing."); + throw new Error("ABsmartly Context is finalizing."); } } @@ -450,7 +494,7 @@ export default class Context { private _checkReady(expectNotFinalized?: boolean) { if (!this.isReady()) { - throw new Error("ABSmartly Context is not yet ready."); + throw new Error("ABsmartly Context is not yet ready."); } if (expectNotFinalized) { @@ -459,6 +503,9 @@ export default class Context { } private _getAttributesMap(): Record { + if (this._attrsMapCache !== null && this._attrsMapCacheSeq === this._attrsSeq) { + return this._attrsMapCache; + } const attrs: Record = {}; if (this._opts.includeSystemAttributes === true) { const client = this._sdk.getClient(); @@ -472,12 +519,23 @@ export default class Context { attrs["app_version"] = app.version; } } - this._attrs.forEach((attr) => { + for (const attr of this._attrs) { attrs[attr.name] = attr.value; - }); + } + this._attrsMapCache = attrs; + this._attrsMapCacheSeq = this._attrsSeq; return attrs; } + private _evaluateAudience(audience: string): boolean | null { + try { + return this._audienceMatcher.evaluate(audience, this._getAttributesMap()); + } catch (error) { + this._logError(error as Error); + return null; + } + } + private _assign(experimentName: string) { const experimentMatches = (experiment: ExperimentData, assignment: Assignment) => { return ( @@ -516,7 +574,7 @@ export default class Context { if (!assignment.ruleOverride && experiment.audience && experiment.audience.length > 0) { const result = this._audienceMatcher.evaluate(experiment.audience, attrs); - const newAudienceMismatch = typeof result === "boolean" ? !result : false; + const newAudienceMismatch = result !== true; if (newAudienceMismatch !== assignment.audienceMismatch) { return false; @@ -606,9 +664,7 @@ export default class Context { if (experiment.data.audience && experiment.data.audience.length > 0) { const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); - if (typeof result === "boolean") { - assignment.audienceMismatch = !result; - } + assignment.audienceMismatch = result !== true; } if (experiment.data.audienceStrict && assignment.audienceMismatch) { @@ -753,14 +809,18 @@ export default class Context { if (field.value === "") return ""; return JSON.parse(field.value); } catch (e) { - console.error(`Failed to parse JSON custom field value '${key}' for experiment '${experimentName}'`); + this._logError(new Error( + `Failed to parse JSON custom field value '${key}' for experiment '${experimentName}': ${(e as Error).message}` + )); return null; } case "boolean": return field.value === "true"; default: - console.error( - `Unknown custom field type '${field.type}' for experiment '${experimentName}' and key '${key}' - you may need to upgrade to the latest SDK version` + this._logError( + new Error( + `Unknown custom field type '${field.type}' for experiment '${experimentName}' and key '${key}' - you may need to upgrade to the latest SDK version` + ) ); return null; } @@ -771,6 +831,12 @@ export default class Context { } customFieldValue(experimentName: string, key: string) { + if (typeof experimentName !== "string" || experimentName.trim().length === 0) { + throw new Error("Experiment name must be a non-empty string"); + } + if (typeof key !== "string" || key.trim().length === 0) { + throw new Error("Field key must be a non-empty string"); + } this._checkReady(true); return this._customFieldValue(experimentName, key); @@ -790,19 +856,24 @@ export default class Context { } customFieldValueType(experimentName: string, key: string) { + if (typeof experimentName !== "string" || experimentName.trim().length === 0) { + throw new Error("Experiment name must be a non-empty string"); + } + if (typeof key !== "string" || key.trim().length === 0) { + throw new Error("Field key must be a non-empty string"); + } this._checkReady(true); return this._customFieldValueType(experimentName, key); } - private _variableValue(key: string, defaultValue: string): string { - for (const i in this._indexVariables[key]) { - const experimentName = this._indexVariables[key][i].data.name; + private _resolveVariableValue(key: string, defaultValue: string, shouldQueueExposure: boolean): string { + for (const experiment of this._indexVariables[key] ?? []) { + const experimentName = experiment.data.name; const assignment = this._assign(experimentName); if (assignment.variables !== undefined) { - if (!assignment.exposed) { + if (shouldQueueExposure && !assignment.exposed) { assignment.exposed = true; - this._queueExposure(experimentName, assignment); } @@ -815,18 +886,12 @@ export default class Context { return defaultValue; } - private _peekVariable(key: string, defaultValue: string): string { - for (const i in this._indexVariables[key]) { - const experimentName = this._indexVariables[key][i].data.name; - const assignment = this._assign(experimentName); - if (assignment.variables !== undefined) { - if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { - return assignment.variables[key] as string; - } - } - } + private _variableValue(key: string, defaultValue: string): string { + return this._resolveVariableValue(key, defaultValue, true); + } - return defaultValue; + private _peekVariable(key: string, defaultValue: string): string { + return this._resolveVariableValue(key, defaultValue, false); } private _validateGoal(goalName: string, properties?: Record) { @@ -855,8 +920,15 @@ export default class Context { private _setTimeout() { if (this.isReady()) { if (this._publishTimeout === undefined && this._opts.publishDelay >= 0) { - this._publishTimeout = setTimeout(() => { - this._flush(); + this._publishTimeout = setTimeout(async () => { + try { + await new Promise((resolve, reject) => { + this._flush((error?: Error) => { + if (error) reject(error); + else resolve(); + }); + }); + } catch {} }, this._opts.publishDelay); } } @@ -943,6 +1015,19 @@ export default class Context { request.attributes = allAttributes; } + // Snapshot and reset synchronously before the async publish. + // The data is already copied into `request` via .map(), so clearing + // immediately is safe and allows new events to accumulate during the + // in-flight publish. On failure, we restore the snapshot so the events + // are retried on the next flush cycle. + const pendingCount = this._pending; + const pendingExposures = this._exposures; + const pendingGoals = this._goals; + + this._pending = 0; + this._exposures = []; + this._goals = []; + this._publisher .publish(request, this._sdk, this, requestOptions) .then(() => { @@ -953,6 +1038,10 @@ export default class Context { } }) .catch((e: Error) => { + this._pending += pendingCount; + this._exposures.push(...pendingExposures); + this._goals.push(...pendingGoals); + this._logError(e); if (typeof callback === "function") { @@ -967,14 +1056,18 @@ export default class Context { } } } else { + this._logError(new Error( + `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` + )); + + this._pending = 0; + this._exposures = []; + this._goals = []; + if (typeof callback === "function") { callback(); } } - - this._pending = 0; - this._exposures = []; - this._goals = []; } } @@ -1038,7 +1131,7 @@ export default class Context { const index: Record = {}; const indexVariables: Record = {}; - (data.experiments || []).forEach((experiment) => { + for (const experiment of data.experiments || []) { const variables: Record[] = []; const entry = { data: experiment, @@ -1047,11 +1140,25 @@ export default class Context { index[experiment.name] = entry; - experiment.variants.forEach((variant, i) => { + for (let i = 0; i < experiment.variants.length; i++) { + const variant = experiment.variants[i]; const config = variant.config; - const parsed = config != null && config.length > 0 ? JSON.parse(config) : {}; + let parsed = {}; - Object.keys(parsed).forEach((key) => { + if (config != null && config.length > 0) { + try { + const value = JSON.parse(config); + if (isObject(value)) { + parsed = value; + } + } catch (error) { + this._logError(new Error( + `Failed to parse config for experiment '${experiment.name}' variant ${i}: ${(error as Error).message}` + )); + } + } + + for (const key of Object.keys(parsed)) { const value = entry; if (indexVariables[key]) { insertUniqueSorted( @@ -1060,18 +1167,27 @@ export default class Context { (a, b) => (a as Experiment).data.id < (b as Experiment).data.id ); } else indexVariables[key] = [value]; - }); + } variables[i] = parsed; - }); - }); + } + } this._index = index; this._indexVariables = indexVariables; this._assignments = assignments; if (!this._failed && this._opts.refreshPeriod > 0 && !this._refreshInterval) { - this._refreshInterval = setInterval(() => this._refresh(), this._opts.refreshPeriod); + this._refreshInterval = setInterval(async () => { + try { + await new Promise((resolve, reject) => { + this._refresh((error?: Error) => { + if (error) reject(error); + else resolve(); + }); + }); + } catch {} + }, this._opts.refreshPeriod); } } diff --git a/src/fetch.ts b/src/fetch.ts index 088f22e..fcbbba7 100644 --- a/src/fetch.ts +++ b/src/fetch.ts @@ -1,26 +1,52 @@ import { isLongLivedApp, isWorker } from "./utils"; import fetchShim from "./fetch-shim"; -const exported = isLongLivedApp() - ? window.fetch - ? window.fetch.bind(window) - : fetchShim - : isWorker() - ? self.fetch - ? self.fetch.bind(self) - : fetchShim - : global - ? global.fetch - ? global.fetch.bind(global) - : function (url: string, opts: Record) { - return new Promise((resolve, reject) => { - import("node-fetch") - .then((fetchNode) => { - fetchNode.default(url.replace(/^\/\//g, "https://"), opts).then(resolve).catch(reject); - }) - .catch(reject); - }); - } - : undefined; +function getFetchImplementation() { + if (isLongLivedApp()) { + if (window.fetch) { + return window.fetch.bind(window); + } + return fetchShim; + } + + if (isWorker()) { + if (self.fetch) { + return self.fetch.bind(self); + } + return fetchShim; + } + + const globalObj = + typeof globalThis !== "undefined" + ? globalThis + : typeof global !== "undefined" + ? global + : undefined; + + if (globalObj !== undefined) { + if ((globalObj as Record).fetch) { + return ((globalObj as Record).fetch as Function).bind(globalObj); + } + return function (url: string, opts: Record) { + return new Promise((resolve, reject) => { + import("node-fetch") + .then((fetchNode) => { + fetchNode.default(url.replace(/^\/\//g, "https://"), opts).then(resolve).catch(reject); + }) + .catch(reject); + }); + }; + } + + return undefined; +} + +const impl = getFetchImplementation(); + +const exported = + impl ?? + function () { + throw new Error("No fetch implementation found. Please provide a fetch polyfill for your environment."); + }; export default exported; diff --git a/src/index.ts b/src/index.ts index aa43221..8f81ea2 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,5 +6,5 @@ import { ContextPublisher } from "./publisher"; // eslint-disable-next-line no-shadow import { AbortController } from "./abort"; -export { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK }; -export default { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK }; +export { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, SDK as Absmartly }; +export default { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, Absmartly: SDK }; diff --git a/src/jsonexpr/operators/eq.ts b/src/jsonexpr/operators/eq.ts index bc71f8c..225b7ee 100644 --- a/src/jsonexpr/operators/eq.ts +++ b/src/jsonexpr/operators/eq.ts @@ -2,6 +2,12 @@ import { Evaluator } from "../evaluator"; import { BinaryOperator } from "./binary"; export class EqualsOperator extends BinaryOperator { + evaluate(evaluator: Evaluator, args: unknown[]) { + const lhs = args.length > 0 ? evaluator.evaluate(args[0]) : null; + const rhs = args.length > 1 ? evaluator.evaluate(args[1]) : null; + return this.binary(evaluator, lhs, rhs); + } + binary(evaluator: Evaluator, lhs: unknown, rhs: unknown) { const result = evaluator.compare(lhs, rhs); return result !== null ? result === 0 : null; diff --git a/src/jsonexpr/operators/match.ts b/src/jsonexpr/operators/match.ts index a18be52..bc4db32 100644 --- a/src/jsonexpr/operators/match.ts +++ b/src/jsonexpr/operators/match.ts @@ -1,16 +1,53 @@ import { Evaluator } from "../evaluator"; import { BinaryOperator } from "./binary"; +const MAX_PATTERN_LENGTH = 1000; +const MAX_TEXT_LENGTH = 10000; +const MAX_REGEX_CACHE_SIZE = 100; + +const REDOS_PATTERN = /(\+|\*|\{)\)(\+|\*|\{)/; + +function hasNestedQuantifiers(pattern: string): boolean { + return REDOS_PATTERN.test(pattern); +} + +const regexCache = new Map(); + +function getOrCompileRegex(pattern: string): RegExp { + let compiled = regexCache.get(pattern); + if (compiled !== undefined) { + return compiled; + } + compiled = new RegExp(pattern); + if (regexCache.size >= MAX_REGEX_CACHE_SIZE) { + const firstKey = regexCache.keys().next().value; + if (firstKey !== undefined) { + regexCache.delete(firstKey); + } + } + regexCache.set(pattern, compiled); + return compiled; +} + export class MatchOperator extends BinaryOperator { binary(evaluator: Evaluator, text: string | null, pattern: string | null) { text = evaluator.stringConvert(text); if (text !== null) { pattern = evaluator.stringConvert(pattern); if (pattern !== null) { + if (pattern.length > MAX_PATTERN_LENGTH) { + return null; + } + if (text.length > MAX_TEXT_LENGTH) { + return null; + } + if (hasNestedQuantifiers(pattern)) { + return null; + } try { - const compiled = new RegExp(pattern); + const compiled = getOrCompileRegex(pattern); return compiled.test(text); - } catch (ignored) { + } catch (error) { return null; } } diff --git a/src/matcher.ts b/src/matcher.ts index eb1c015..47287e1 100644 --- a/src/matcher.ts +++ b/src/matcher.ts @@ -3,15 +3,17 @@ import { JsonExpr } from "./jsonexpr/jsonexpr"; export class AudienceMatcher { evaluate(audienceString: string, vars: Record) { + let audience; try { - const audience = JSON.parse(audienceString); - if (audience && audience.filter) { - if (Array.isArray(audience.filter) || isObject(audience.filter)) { - return this._jsonExpr.evaluateBooleanExpr(audience.filter, vars); - } + audience = JSON.parse(audienceString); + } catch (_error) { + return null; + } + + if (audience && audience.filter) { + if (Array.isArray(audience.filter) || isObject(audience.filter)) { + return this._jsonExpr.evaluateBooleanExpr(audience.filter, vars); } - } catch (e) { - console.error(e); } return null; diff --git a/src/provider.ts b/src/provider.ts index 30dac20..85c14c8 100644 --- a/src/provider.ts +++ b/src/provider.ts @@ -1,5 +1,5 @@ import SDK from "./sdk"; -import { ClientRequestOptions } from "./client"; +import { type ClientRequestOptions } from "./client"; export class ContextDataProvider { getContextData(sdk: SDK, requestOptions?: Partial) { diff --git a/src/publisher.ts b/src/publisher.ts index 79c5f14..8d6f890 100644 --- a/src/publisher.ts +++ b/src/publisher.ts @@ -1,6 +1,6 @@ -import Context, { Attribute, Exposure, Goal, Unit } from "./context"; +import Context, { type Attribute, type Exposure, type Goal, type Unit } from "./context"; import SDK from "./sdk"; -import { ClientRequestOptions } from "./client"; +import { type ClientRequestOptions } from "./client"; export type PublishParams = { units: Unit[]; diff --git a/src/sdk.ts b/src/sdk.ts index 32ac4cf..2f97ea7 100644 --- a/src/sdk.ts +++ b/src/sdk.ts @@ -1,6 +1,12 @@ -import Client, { ClientOptions, ClientRequestOptions } from "./client"; -import Context, { ContextData, ContextOptions, ContextParams, Exposure, Goal } from "./context"; -import { ContextPublisher, PublishParams } from "./publisher"; +import Client, { type ClientOptions, type ClientRequestOptions } from "./client"; +import Context, { + type ContextData, + type ContextOptions, + type ContextParams, + type Exposure, + type Goal, +} from "./context"; +import { ContextPublisher, type PublishParams } from "./publisher"; import { ContextDataProvider } from "./provider"; import { isLongLivedApp } from "./utils"; @@ -29,20 +35,7 @@ export default class SDK { private readonly _client: Client; constructor(options: ClientOptions & SDKOptions) { - const clientOptions = Object.assign( - { - agent: "absmartly-javascript-sdk", - }, - ...Object.entries(options || {}) - .filter( - (x) => - ["application", "agent", "apiKey", "endpoint", "keepalive", "environment", "retries", "timeout"].indexOf( - x[0] - ) !== -1 - ) - .map((x) => ({ [x[0]]: x[1] })) - ); - + const clientOptions = SDK._extractClientOptions(options); options = Object.assign({}, options); this._client = options.client || new Client(clientOptions); @@ -51,6 +44,30 @@ export default class SDK { this._provider = options.provider || new ContextDataProvider(); } + private static _extractClientOptions(options: ClientOptions & SDKOptions): ClientOptions { + const clientOptionKeys = [ + "application", + "agent", + "apiKey", + "endpoint", + "keepalive", + "environment", + "retries", + "timeout", + ]; + const extracted: Partial = { + agent: "absmartly-javascript-sdk", + }; + + for (const [key, value] of Object.entries(options || {})) { + if (clientOptionKeys.includes(key)) { + (extracted as Record)[key] = value; + } + } + + return extracted as ClientOptions; + } + getContextData(requestOptions: ClientRequestOptions) { return this._provider.getContextData(this, requestOptions); } @@ -108,29 +125,31 @@ export default class SDK { } private static _contextOptions(options?: Partial): ContextOptions { - return Object.assign( - { - publishDelay: isLongLivedApp() ? 100 : -1, - refreshPeriod: 0, - }, - options || {} - ); + const DEFAULT_PUBLISH_DELAY_MS = 100; + const NO_PUBLISH_DELAY = -1; + const NO_REFRESH = 0; + + return { + publishDelay: isLongLivedApp() ? DEFAULT_PUBLISH_DELAY_MS : NO_PUBLISH_DELAY, + refreshPeriod: NO_REFRESH, + ...options, + }; } private static _validateParams(params: ContextParams) { - Object.entries(params.units).forEach((entry) => { - const type = typeof entry[1]; + for (const [unitType, uid] of Object.entries(params.units)) { + const type = typeof uid; if (type !== "string" && type !== "number") { throw new Error( - `Unit '${entry[0]}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']` + `Unit '${unitType}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']` ); } - if (typeof entry[1] === "string") { - if (entry[1].length === 0) { - throw new Error(`Unit '${entry[0]}' UID length must be >= 1`); + if (typeof uid === "string") { + if (uid.length === 0) { + throw new Error(`Unit '${unitType}' UID length must be >= 1`); } } - }); + } } } diff --git a/src/utils.ts b/src/utils.ts index 529d486..512089f 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -149,24 +149,31 @@ export function arrayEqualsShallow(a?: unknown[], b?: unknown[]) { } export function stringToUint8Array(value: string) { - const n = value.length; - const array = new Array(value.length); + if (typeof TextEncoder !== "undefined") { + return new TextEncoder().encode(value); + } - let k = 0; - for (let i = 0; i < n; ++i) { - const c = value.charCodeAt(i); + const utf8: number[] = []; + for (let i = 0; i < value.length; i++) { + let c = value.charCodeAt(i); + if (c >= 0xd800 && c <= 0xdbff && i + 1 < value.length) { + const next = value.charCodeAt(i + 1); + if (next >= 0xdc00 && next <= 0xdfff) { + c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000; + i++; + } + } if (c < 0x80) { - array[k++] = c; + utf8.push(c); } else if (c < 0x800) { - array[k++] = (c >> 6) | 192; - array[k++] = (c & 63) | 128; + utf8.push(0xc0 | (c >> 6), 0x80 | (c & 0x3f)); + } else if (c < 0x10000) { + utf8.push(0xe0 | (c >> 12), 0x80 | ((c >> 6) & 0x3f), 0x80 | (c & 0x3f)); } else { - array[k++] = (c >> 12) | 224; - array[k++] = ((c >> 6) & 63) | 128; - array[k++] = (c & 63) | 128; + utf8.push(0xf0 | (c >> 18), 0x80 | ((c >> 12) & 0x3f), 0x80 | ((c >> 6) & 0x3f), 0x80 | (c & 0x3f)); } } - return Uint8Array.from(array); + return new Uint8Array(utf8); } const Base64URLNoPaddingChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_"; From 64eaaba56a05c4d5b35fb907464b55e517d13f00 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 30 Apr 2026 11:01:16 +0100 Subject: [PATCH 02/25] fix: address PR review comments - Add ReDoS protection in match.ts using safe-regex2 (replaces narrow custom regex check that only caught a few quantifier shapes) - Move SDK/Client constructor-merge tests to fixes-constructors.test.js so they exercise real classes instead of Jest doubles (file-scope jest.mock("../client"|"../sdk") in fixes.test.js made the previous versions vacuous) - Apply fail-closed semantics to audience evaluation: route both call sites through _evaluateAudience() and treat any non-true result as a mismatch, so audienceStrict still protects against malformed audiences - Update one stale exposure expectation in context.test.js to include ruleOverride and sdkVersion (post-rebase) - Document the empty publishDelay/refreshPeriod catch blocks (the callbacks already log via _logError) and replace `Function` cast in fetch.ts with a typed signature --- package-lock.json | 47 ++++++++++++- package.json | 3 +- src/__tests__/context.test.js | 3 + src/__tests__/fixes-constructors.test.js | 86 ++++++++++++++++++++++++ src/__tests__/fixes.test.js | 45 +------------ src/context.ts | 38 +++++++---- src/fetch.ts | 9 +-- src/jsonexpr/operators/match.ts | 12 ++-- 8 files changed, 172 insertions(+), 71 deletions(-) create mode 100644 src/__tests__/fixes-constructors.test.js diff --git a/package-lock.json b/package-lock.json index 50528ba..4816113 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,7 +12,8 @@ "@babel/runtime": "^7.29.2", "core-js": "^3.20.0", "node-fetch": "^2.6.7", - "rfdc": "^1.3.0" + "rfdc": "^1.3.0", + "safe-regex2": "^5.1.1" }, "devDependencies": { "@babel/cli": "^7.17.3", @@ -8744,6 +8745,15 @@ "url": "https://github.com/sponsors/isaacs" } }, + "node_modules/ret": { + "version": "0.5.0", + "resolved": "https://registry.npmjs.org/ret/-/ret-0.5.0.tgz", + "integrity": "sha512-I1XxrZSQ+oErkRR4jYbAyEEu2I0avBvvMM5JN+6EBprOGRCs63ENqZ3vjavq8fBw2+62G5LF5XelKwuJpcvcxw==", + "license": "MIT", + "engines": { + "node": ">=10" + } + }, "node_modules/reusify": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/reusify/-/reusify-1.0.4.tgz", @@ -8818,6 +8828,28 @@ } ] }, + "node_modules/safe-regex2": { + "version": "5.1.1", + "resolved": "https://registry.npmjs.org/safe-regex2/-/safe-regex2-5.1.1.tgz", + "integrity": "sha512-mOSBvHGDZMuIEZMdOz/aCEYDCv0E7nfcNsIhUF+/P+xC7Hyf3FkvymqgPbg9D1EdSGu+uKbJgy09K/RKKc7kJA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/fastify" + }, + { + "type": "opencollective", + "url": "https://opencollective.com/fastify" + } + ], + "license": "MIT", + "dependencies": { + "ret": "~0.5.0" + }, + "bin": { + "safe-regex2": "bin/safe-regex2.js" + } + }, "node_modules/schema-utils": { "version": "2.7.1", "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-2.7.1.tgz", @@ -16502,6 +16534,11 @@ } } }, + "ret": { + "version": "0.5.0", + "resolved": "https://registry.npmjs.org/ret/-/ret-0.5.0.tgz", + "integrity": "sha512-I1XxrZSQ+oErkRR4jYbAyEEu2I0avBvvMM5JN+6EBprOGRCs63ENqZ3vjavq8fBw2+62G5LF5XelKwuJpcvcxw==" + }, "reusify": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/reusify/-/reusify-1.0.4.tgz", @@ -16537,6 +16574,14 @@ "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==", "dev": true }, + "safe-regex2": { + "version": "5.1.1", + "resolved": "https://registry.npmjs.org/safe-regex2/-/safe-regex2-5.1.1.tgz", + "integrity": "sha512-mOSBvHGDZMuIEZMdOz/aCEYDCv0E7nfcNsIhUF+/P+xC7Hyf3FkvymqgPbg9D1EdSGu+uKbJgy09K/RKKc7kJA==", + "requires": { + "ret": "~0.5.0" + } + }, "schema-utils": { "version": "2.7.1", "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-2.7.1.tgz", diff --git a/package.json b/package.json index 220053b..1fbb7c8 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,8 @@ "@babel/runtime": "^7.29.2", "core-js": "^3.20.0", "node-fetch": "^2.6.7", - "rfdc": "^1.3.0" + "rfdc": "^1.3.0", + "safe-regex2": "^5.1.1" }, "devDependencies": { "@babel/cli": "^7.17.3", diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 6d0a34e..b30a472 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4538,6 +4538,7 @@ describe("Context", () => { publishedAt: 1611141535729, units: publishUnits, hashed: true, + sdkVersion: SDK_VERSION, exposures: [ { id: 1, @@ -4551,6 +4552,7 @@ describe("Context", () => { fullOn: false, custom: false, audienceMismatch: false, + ruleOverride: false, }, { id: 1, @@ -4564,6 +4566,7 @@ describe("Context", () => { fullOn: false, custom: false, audienceMismatch: false, + ruleOverride: false, }, ], }, diff --git a/src/__tests__/fixes-constructors.test.js b/src/__tests__/fixes-constructors.test.js new file mode 100644 index 0000000..5a30e56 --- /dev/null +++ b/src/__tests__/fixes-constructors.test.js @@ -0,0 +1,86 @@ +// Constructor-merge tests for SDK and Client. Kept in a separate file from +// fixes.test.js so the file-scope jest.mock("../client") / jest.mock("../sdk") +// in that file doesn't replace these constructors with Jest doubles, which +// would make these assertions vacuous (passing without exercising the real +// _extractClientOptions / option-merge logic). + +import SDK from "../sdk"; +import Client from "../client"; + +describe("Fix #11: SDK._extractClientOptions uses includes", () => { + it("should extract client options and pass them to the real Client", () => { + const sdk = new SDK({ + agent: "test-agent", + apiKey: "key", + application: "app", + endpoint: "http://localhost", + environment: "test", + timeout: 5000, + }); + + const client = sdk.getClient(); + expect(client).toBeInstanceOf(Client); + expect(client.getAgent()).toBe("test-agent"); + expect(client.getEnvironment()).toBe("test"); + expect(client.getApplication()).toEqual({ name: "app", version: 0 }); + }); +}); + +describe("Fix #33: Client constructor uses spread", () => { + it("should merge defaults with provided options and apply them", () => { + const client = new Client({ + endpoint: "http://test", + agent: "custom-agent", + environment: "prod", + apiKey: "key123", + application: "myapp", + timeout: 10000, + }); + + expect(client.getAgent()).toBe("custom-agent"); + expect(client.getEnvironment()).toBe("prod"); + expect(client.getApplication()).toEqual({ name: "myapp", version: 0 }); + }); + + it("should use defaults when optional fields are omitted", () => { + const client = new Client({ + endpoint: "http://test", + environment: "prod", + apiKey: "key123", + application: "myapp", + }); + + expect(client.getAgent()).toBe("javascript-client"); + }); +}); + +describe("Fix #34: SDK._contextOptions uses spread", () => { + it("should merge custom options with defaults via the real constructor", () => { + const sdk = new SDK({ + agent: "test", + apiKey: "key", + application: "app", + endpoint: "http://localhost", + environment: "test", + retries: 2, + timeout: 1234, + }); + + const client = sdk.getClient(); + expect(client).toBeInstanceOf(Client); + expect(client.getAgent()).toBe("test"); + expect(client.getEnvironment()).toBe("test"); + }); + + it("should accept application as an object", () => { + const sdk = new SDK({ + agent: "test", + apiKey: "key", + application: { name: "myapp", version: "1.2.3" }, + endpoint: "http://localhost", + environment: "prod", + }); + + expect(sdk.getClient().getApplication()).toEqual({ name: "myapp", version: "1.2.3" }); + }); +}); diff --git a/src/__tests__/fixes.test.js b/src/__tests__/fixes.test.js index 1cf08d8..f18c77e 100644 --- a/src/__tests__/fixes.test.js +++ b/src/__tests__/fixes.test.js @@ -279,19 +279,9 @@ describe("Fix #5: _finalizing type cleanup", () => { }); }); -describe("Fix #11: SDK._extractClientOptions uses includes", () => { - it("should extract client options correctly", () => { - const sdk = new SDK({ - agent: "test-agent", - apiKey: "key", - application: "app", - endpoint: "http://localhost", - environment: "test", - timeout: 5000, - }); - expect(sdk).toBeInstanceOf(SDK); - }); -}); +// Fix #11, #33, #34: real-constructor tests live in fixes-constructors.test.js +// (file-scope jest.mock("../client") and jest.mock("../sdk") in this file +// would otherwise make those tests vacuous against Jest doubles). describe("Fix #12: SDK.defaultEventLogger logs error message", () => { let ActualSDK; @@ -427,35 +417,6 @@ describe("Fix #32: AbortSignal dispatchEvent uses explicit onabort", () => { }); }); -describe("Fix #33: Client constructor uses spread", () => { - it("should merge defaults with provided options", () => { - const client = new Client({ - endpoint: "http://test", - agent: "custom-agent", - environment: "prod", - apiKey: "key123", - application: "myapp", - timeout: 10000, - }); - - expect(client).toBeInstanceOf(Client); - }); -}); - -describe("Fix #34: SDK._contextOptions uses spread", () => { - it("should merge custom options with defaults", () => { - const sdk = new SDK({ - agent: "test", - apiKey: "key", - application: "app", - endpoint: "http://localhost", - environment: "test", - }); - - expect(sdk).toBeInstanceOf(SDK); - }); -}); - describe("Fix #20: EqualsOperator without redundant Array.isArray", () => { const operator = new EqualsOperator(); const evaluator = mockEvaluator(); diff --git a/src/context.ts b/src/context.ts index 73304b4..6186b1b 100644 --- a/src/context.ts +++ b/src/context.ts @@ -573,7 +573,7 @@ export default class Context { } if (!assignment.ruleOverride && experiment.audience && experiment.audience.length > 0) { - const result = this._audienceMatcher.evaluate(experiment.audience, attrs); + const result = this._evaluateAudience(experiment.audience); const newAudienceMismatch = result !== true; if (newAudienceMismatch !== assignment.audienceMismatch) { @@ -662,7 +662,7 @@ export default class Context { assignment.ruleOverride = true; } else { if (experiment.data.audience && experiment.data.audience.length > 0) { - const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); + const result = this._evaluateAudience(experiment.data.audience); assignment.audienceMismatch = result !== true; } @@ -809,9 +809,13 @@ export default class Context { if (field.value === "") return ""; return JSON.parse(field.value); } catch (e) { - this._logError(new Error( - `Failed to parse JSON custom field value '${key}' for experiment '${experimentName}': ${(e as Error).message}` - )); + this._logError( + new Error( + `Failed to parse JSON custom field value '${key}' for experiment '${experimentName}': ${ + (e as Error).message + }` + ) + ); return null; } case "boolean": @@ -928,7 +932,9 @@ export default class Context { else resolve(); }); }); - } catch {} + } catch { + // _flush already logs publish errors via the callback. + } }, this._opts.publishDelay); } } @@ -1056,9 +1062,11 @@ export default class Context { } } } else { - this._logError(new Error( - `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` - )); + this._logError( + new Error( + `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` + ) + ); this._pending = 0; this._exposures = []; @@ -1152,9 +1160,11 @@ export default class Context { parsed = value; } } catch (error) { - this._logError(new Error( - `Failed to parse config for experiment '${experiment.name}' variant ${i}: ${(error as Error).message}` - )); + this._logError( + new Error( + `Failed to parse config for experiment '${experiment.name}' variant ${i}: ${(error as Error).message}` + ) + ); } } @@ -1186,7 +1196,9 @@ export default class Context { else resolve(); }); }); - } catch {} + } catch { + // _refresh already logs refresh errors via the callback. + } }, this._opts.refreshPeriod); } } diff --git a/src/fetch.ts b/src/fetch.ts index fcbbba7..149277f 100644 --- a/src/fetch.ts +++ b/src/fetch.ts @@ -16,16 +16,11 @@ function getFetchImplementation() { return fetchShim; } - const globalObj = - typeof globalThis !== "undefined" - ? globalThis - : typeof global !== "undefined" - ? global - : undefined; + const globalObj = typeof globalThis !== "undefined" ? globalThis : typeof global !== "undefined" ? global : undefined; if (globalObj !== undefined) { if ((globalObj as Record).fetch) { - return ((globalObj as Record).fetch as Function).bind(globalObj); + return ((globalObj as Record).fetch as (...args: unknown[]) => unknown).bind(globalObj); } return function (url: string, opts: Record) { return new Promise((resolve, reject) => { diff --git a/src/jsonexpr/operators/match.ts b/src/jsonexpr/operators/match.ts index bc4db32..31bfc4b 100644 --- a/src/jsonexpr/operators/match.ts +++ b/src/jsonexpr/operators/match.ts @@ -1,3 +1,5 @@ +import safeRegex from "safe-regex2"; + import { Evaluator } from "../evaluator"; import { BinaryOperator } from "./binary"; @@ -5,12 +7,6 @@ const MAX_PATTERN_LENGTH = 1000; const MAX_TEXT_LENGTH = 10000; const MAX_REGEX_CACHE_SIZE = 100; -const REDOS_PATTERN = /(\+|\*|\{)\)(\+|\*|\{)/; - -function hasNestedQuantifiers(pattern: string): boolean { - return REDOS_PATTERN.test(pattern); -} - const regexCache = new Map(); function getOrCompileRegex(pattern: string): RegExp { @@ -41,7 +37,9 @@ export class MatchOperator extends BinaryOperator { if (text.length > MAX_TEXT_LENGTH) { return null; } - if (hasNestedQuantifiers(pattern)) { + // Reject patterns vulnerable to catastrophic backtracking (ReDoS). + // Length caps alone do not prevent attacks like (a+)+ or (a|a)+. + if (!safeRegex(pattern)) { return null; } try { From b1b62cdfd7dbce634c68f64343101bd52c419093 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 30 Apr 2026 11:17:21 +0100 Subject: [PATCH 03/25] fix: drop async setTimeout/setInterval to avoid regenerator-runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The async/await wrappers in _setTimeout/_setInterval scheduled the callback-based _flush/_refresh just to swallow errors that those methods already log internally. Calling them directly drops the dead try/await wrapping and removes the regenerator-runtime polyfill that babel was injecting for the IE10 browser target (which broke build-browser). Also stop destructuring `agent` into `_` in client.test.js — replace with delete to clear the no-unused-vars warning. --- src/__tests__/client.test.js | 3 ++- src/context.ts | 28 ++++++---------------------- 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/__tests__/client.test.js b/src/__tests__/client.test.js index fca41fd..d868fc5 100644 --- a/src/__tests__/client.test.js +++ b/src/__tests__/client.test.js @@ -1121,7 +1121,8 @@ describe("Client", () => { }); it("getAgent() should return default agent when not specified", () => { - const { agent: _, ...optionsWithoutAgent } = clientOptions; + const optionsWithoutAgent = { ...clientOptions }; + delete optionsWithoutAgent.agent; const client = new Client(optionsWithoutAgent); expect(client.getAgent()).toEqual("javascript-client"); }); diff --git a/src/context.ts b/src/context.ts index 6186b1b..aaa9fd7 100644 --- a/src/context.ts +++ b/src/context.ts @@ -924,17 +924,9 @@ export default class Context { private _setTimeout() { if (this.isReady()) { if (this._publishTimeout === undefined && this._opts.publishDelay >= 0) { - this._publishTimeout = setTimeout(async () => { - try { - await new Promise((resolve, reject) => { - this._flush((error?: Error) => { - if (error) reject(error); - else resolve(); - }); - }); - } catch { - // _flush already logs publish errors via the callback. - } + this._publishTimeout = setTimeout(() => { + // _flush already logs publish errors via the callback. + this._flush(); }, this._opts.publishDelay); } } @@ -1188,17 +1180,9 @@ export default class Context { this._assignments = assignments; if (!this._failed && this._opts.refreshPeriod > 0 && !this._refreshInterval) { - this._refreshInterval = setInterval(async () => { - try { - await new Promise((resolve, reject) => { - this._refresh((error?: Error) => { - if (error) reject(error); - else resolve(); - }); - }); - } catch { - // _refresh already logs refresh errors via the callback. - } + this._refreshInterval = setInterval(() => { + // _refresh already logs refresh errors via the callback. + this._refresh(); }, this._opts.refreshPeriod); } } From 5bd48d8ef839e331d44c28c708d7ed5a60c696f7 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Tue, 16 Jun 2026 15:09:57 +0100 Subject: [PATCH 04/25] fix(jsonexpr): eq(null, null) returns null (canonical null-operand handling) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A null operand short-circuits binary operators to null — eq(null, null) is null, not true. This matches origin/main of every SDK and the collector. The branch had introduced an eq override (or removed the base null-skip) that made eq(null,null) true, diverging from the canonical behavior. Revert to the null-skip behavior and align the operator tests. --- src/__tests__/jsonexpr/operators/eq.test.js | 6 ++---- src/jsonexpr/operators/eq.ts | 6 ------ 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/__tests__/jsonexpr/operators/eq.test.js b/src/__tests__/jsonexpr/operators/eq.test.js index 82d507d..d7132af 100644 --- a/src/__tests__/jsonexpr/operators/eq.test.js +++ b/src/__tests__/jsonexpr/operators/eq.test.js @@ -39,11 +39,9 @@ describe("EqOperator", () => { evaluator.compare.mockClear(); expect(operator.evaluate(evaluator, [null, null])).toBe(null); - expect(evaluator.evaluate).toHaveBeenCalledTimes(2); + expect(evaluator.evaluate).toHaveBeenCalledTimes(1); expect(evaluator.evaluate).toHaveBeenNthCalledWith(1, null); - expect(evaluator.evaluate).toHaveBeenNthCalledWith(2, null); - expect(evaluator.compare).toHaveBeenCalledTimes(1); - expect(evaluator.compare).toHaveBeenCalledWith(null, null); + expect(evaluator.compare).toHaveBeenCalledTimes(0); evaluator.evaluate.mockClear(); evaluator.compare.mockClear(); diff --git a/src/jsonexpr/operators/eq.ts b/src/jsonexpr/operators/eq.ts index 225b7ee..bc71f8c 100644 --- a/src/jsonexpr/operators/eq.ts +++ b/src/jsonexpr/operators/eq.ts @@ -2,12 +2,6 @@ import { Evaluator } from "../evaluator"; import { BinaryOperator } from "./binary"; export class EqualsOperator extends BinaryOperator { - evaluate(evaluator: Evaluator, args: unknown[]) { - const lhs = args.length > 0 ? evaluator.evaluate(args[0]) : null; - const rhs = args.length > 1 ? evaluator.evaluate(args[1]) : null; - return this.binary(evaluator, lhs, rhs); - } - binary(evaluator: Evaluator, lhs: unknown, rhs: unknown) { const result = evaluator.compare(lhs, rhs); return result !== null ? result === 0 : null; From 627be806372ae9e7dda5c75638ebab3c3391fca5 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Tue, 16 Jun 2026 23:30:31 +0100 Subject: [PATCH 05/25] fix: audienceMismatch stays false for null audience eval; override allowed post-finalize - Audience matching: only set audienceMismatch when the audience evaluates to a boolean. A null result (e.g. an audience like '{}' with no usable filter) must leave audienceMismatch false, matching the collector (ContextAPI: 'if (result != null) audienceMismatch = !result.get()'). The cache-validity re-evaluation uses the same null-guarded logic so a null result no longer needlessly invalidates a cached assignment. - override(): allow after finalize() (parity with the production SDK, which sets overrides unconditionally). Keep the input-type validation but drop the erroneous _checkNotFinalized() guard. Fixes cross-SDK scenarios 13 (Not Eligible - Traffic Split) and 190 (Post-Finalize override allowed). --- src/context.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/context.ts b/src/context.ts index aaa9fd7..95c9c07 100644 --- a/src/context.ts +++ b/src/context.ts @@ -439,7 +439,8 @@ export default class Context { if (typeof variant !== "number" || variant < 0 || !Number.isInteger(variant)) { throw new Error("Variant must be a non-negative integer"); } - this._checkNotFinalized(); + // override() is allowed after finalize() (parity with the production SDK, + // which sets overrides unconditionally). this._overrides = Object.assign(this._overrides, { [experimentName]: variant }); } @@ -574,7 +575,10 @@ export default class Context { if (!assignment.ruleOverride && experiment.audience && experiment.audience.length > 0) { const result = this._evaluateAudience(experiment.audience); - const newAudienceMismatch = result !== true; + // Mirror the assignment-time logic: a null result leaves the + // mismatch flag unchanged (false), so the cached assignment + // stays valid rather than being needlessly invalidated. + const newAudienceMismatch = result !== null ? !result : assignment.audienceMismatch; if (newAudienceMismatch !== assignment.audienceMismatch) { return false; @@ -664,7 +668,13 @@ export default class Context { if (experiment.data.audience && experiment.data.audience.length > 0) { const result = this._evaluateAudience(experiment.data.audience); - assignment.audienceMismatch = result !== true; + // Only flag a mismatch when the audience actually evaluated + // to a boolean. A null result (e.g. an audience with no + // usable filter like `{}`) leaves audienceMismatch false, + // matching the collector (ContextAPI: `if (result != null)`). + if (result !== null) { + assignment.audienceMismatch = !result; + } } if (experiment.data.audienceStrict && assignment.audienceMismatch) { From f3b298a3e1d105b37a3150e800d8fd9bb0e2d78d Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 17 Jun 2026 10:33:11 +0100 Subject: [PATCH 06/25] revert: drop MATCH ReDoS hardening from this branch The match length-cap/safe-regex/cache hardening changed MATCH edge-case behavior vs the canonical collector and diverged from the other SDKs (inconsistent limits; only a subset hardened). Restore the bare collector-equivalent MatchOperator here and remove the safe-regex2 dependency. The hardening now lives on feat/regex-hardening to land as a coordinated cross-SDK security PR. --- package-lock.json | 47 +-------------------------------- package.json | 3 +-- src/jsonexpr/operators/match.ts | 39 ++------------------------- 3 files changed, 4 insertions(+), 85 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4816113..50528ba 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,8 +12,7 @@ "@babel/runtime": "^7.29.2", "core-js": "^3.20.0", "node-fetch": "^2.6.7", - "rfdc": "^1.3.0", - "safe-regex2": "^5.1.1" + "rfdc": "^1.3.0" }, "devDependencies": { "@babel/cli": "^7.17.3", @@ -8745,15 +8744,6 @@ "url": "https://github.com/sponsors/isaacs" } }, - "node_modules/ret": { - "version": "0.5.0", - "resolved": "https://registry.npmjs.org/ret/-/ret-0.5.0.tgz", - "integrity": "sha512-I1XxrZSQ+oErkRR4jYbAyEEu2I0avBvvMM5JN+6EBprOGRCs63ENqZ3vjavq8fBw2+62G5LF5XelKwuJpcvcxw==", - "license": "MIT", - "engines": { - "node": ">=10" - } - }, "node_modules/reusify": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/reusify/-/reusify-1.0.4.tgz", @@ -8828,28 +8818,6 @@ } ] }, - "node_modules/safe-regex2": { - "version": "5.1.1", - "resolved": "https://registry.npmjs.org/safe-regex2/-/safe-regex2-5.1.1.tgz", - "integrity": "sha512-mOSBvHGDZMuIEZMdOz/aCEYDCv0E7nfcNsIhUF+/P+xC7Hyf3FkvymqgPbg9D1EdSGu+uKbJgy09K/RKKc7kJA==", - "funding": [ - { - "type": "github", - "url": "https://github.com/sponsors/fastify" - }, - { - "type": "opencollective", - "url": "https://opencollective.com/fastify" - } - ], - "license": "MIT", - "dependencies": { - "ret": "~0.5.0" - }, - "bin": { - "safe-regex2": "bin/safe-regex2.js" - } - }, "node_modules/schema-utils": { "version": "2.7.1", "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-2.7.1.tgz", @@ -16534,11 +16502,6 @@ } } }, - "ret": { - "version": "0.5.0", - "resolved": "https://registry.npmjs.org/ret/-/ret-0.5.0.tgz", - "integrity": "sha512-I1XxrZSQ+oErkRR4jYbAyEEu2I0avBvvMM5JN+6EBprOGRCs63ENqZ3vjavq8fBw2+62G5LF5XelKwuJpcvcxw==" - }, "reusify": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/reusify/-/reusify-1.0.4.tgz", @@ -16574,14 +16537,6 @@ "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==", "dev": true }, - "safe-regex2": { - "version": "5.1.1", - "resolved": "https://registry.npmjs.org/safe-regex2/-/safe-regex2-5.1.1.tgz", - "integrity": "sha512-mOSBvHGDZMuIEZMdOz/aCEYDCv0E7nfcNsIhUF+/P+xC7Hyf3FkvymqgPbg9D1EdSGu+uKbJgy09K/RKKc7kJA==", - "requires": { - "ret": "~0.5.0" - } - }, "schema-utils": { "version": "2.7.1", "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-2.7.1.tgz", diff --git a/package.json b/package.json index 1fbb7c8..220053b 100644 --- a/package.json +++ b/package.json @@ -44,8 +44,7 @@ "@babel/runtime": "^7.29.2", "core-js": "^3.20.0", "node-fetch": "^2.6.7", - "rfdc": "^1.3.0", - "safe-regex2": "^5.1.1" + "rfdc": "^1.3.0" }, "devDependencies": { "@babel/cli": "^7.17.3", diff --git a/src/jsonexpr/operators/match.ts b/src/jsonexpr/operators/match.ts index 31bfc4b..a18be52 100644 --- a/src/jsonexpr/operators/match.ts +++ b/src/jsonexpr/operators/match.ts @@ -1,51 +1,16 @@ -import safeRegex from "safe-regex2"; - import { Evaluator } from "../evaluator"; import { BinaryOperator } from "./binary"; -const MAX_PATTERN_LENGTH = 1000; -const MAX_TEXT_LENGTH = 10000; -const MAX_REGEX_CACHE_SIZE = 100; - -const regexCache = new Map(); - -function getOrCompileRegex(pattern: string): RegExp { - let compiled = regexCache.get(pattern); - if (compiled !== undefined) { - return compiled; - } - compiled = new RegExp(pattern); - if (regexCache.size >= MAX_REGEX_CACHE_SIZE) { - const firstKey = regexCache.keys().next().value; - if (firstKey !== undefined) { - regexCache.delete(firstKey); - } - } - regexCache.set(pattern, compiled); - return compiled; -} - export class MatchOperator extends BinaryOperator { binary(evaluator: Evaluator, text: string | null, pattern: string | null) { text = evaluator.stringConvert(text); if (text !== null) { pattern = evaluator.stringConvert(pattern); if (pattern !== null) { - if (pattern.length > MAX_PATTERN_LENGTH) { - return null; - } - if (text.length > MAX_TEXT_LENGTH) { - return null; - } - // Reject patterns vulnerable to catastrophic backtracking (ReDoS). - // Length caps alone do not prevent attacks like (a+)+ or (a|a)+. - if (!safeRegex(pattern)) { - return null; - } try { - const compiled = getOrCompileRegex(pattern); + const compiled = new RegExp(pattern); return compiled.test(text); - } catch (error) { + } catch (ignored) { return null; } } From 1dfea8800424f0eaae4bab8b3b5e7b5241875af9 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 17 Jun 2026 10:40:40 +0100 Subject: [PATCH 07/25] fix: correct SDK alias casing to ABsmartly (brand convention) The SDK alias used 'Absmartly' (lowercase b); the rest of the codebase uses 'ABsmartly' (client.ts, context.ts, error messages). Align both the named and default exports. --- src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 8f81ea2..f9fa0ee 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,5 +6,5 @@ import { ContextPublisher } from "./publisher"; // eslint-disable-next-line no-shadow import { AbortController } from "./abort"; -export { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, SDK as Absmartly }; -export default { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, Absmartly: SDK }; +export { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, SDK as ABsmartly }; +export default { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, ABsmartly: SDK }; From 9fa51239518fff0d5dce4112bc9d9a853e6959ab Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 17 Jun 2026 11:23:41 +0100 Subject: [PATCH 08/25] test: add canonical astral/multibyte hashUnit regression test Asserts hashUnit of emoji/CJK units against the shared canonical values, guarding the UTF-8 surrogate-pair encoding (4-byte sequences) across SDKs. --- src/__tests__/utils.test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/__tests__/utils.test.js b/src/__tests__/utils.test.js index 39cf8d4..87af81c 100644 --- a/src/__tests__/utils.test.js +++ b/src/__tests__/utils.test.js @@ -235,6 +235,17 @@ describe("hashUnit()", () => { done(); }); + + it("should hash astral/multibyte characters correctly", (done) => { + // Characters outside the BMP are stored as UTF-16 surrogate pairs and must + // encode to 4-byte UTF-8; these canonical hashes are shared across all SDKs. + expect(hashUnit("😀")).toBe("KgLqw51xanDs83V5GFkntg"); + expect(hashUnit("😀😁")).toBe("ZJuDalvUWRJnVtkspj-2bQ"); + expect(hashUnit("世界你好")).toBe("v2CJG7YcjjWncKOSCzF2GA"); + expect(hashUnit("user_世界_123")).toBe("SCgk4OzXlFMvo1UMsP88fA"); + + done(); + }); }); describe("chooseVariant()", () => { From 2320d54c86b4fd8a7e5d6c65b3b443e92c15c263 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 17 Jun 2026 17:14:20 +0100 Subject: [PATCH 09/25] docs: remove client-side security warning; restore Publishing Pending Data section - Drop the Security Warning: Client-Side Usage block from the README - Restore the publish() intro and example under Publishing Pending Data - De-duplicate the Finalizing section --- README.md | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 1cf3de2..edd7080 100644 --- a/README.md +++ b/README.md @@ -38,23 +38,6 @@ Simply add the following code to your `head` section to include the latest publi ``` -### Security Warning: Client-Side Usage - -**IMPORTANT:** This SDK exposes your API key when used directly in browser environments. API keys should never be embedded in client-side code as they can be extracted from browser bundles or network requests. - -**Recommended Architecture:** -- **DO NOT** use this SDK directly in the browser with your API key -- **DO** use this SDK in Node.js server-side applications -- **DO** create a server-side proxy endpoint that fetches context data on behalf of your frontend -- **DO** use short-lived, per-session tokens instead of API keys for client-side requests - -```text -Browser --> Your Server (with API key) --> ABsmartly API - session token only -``` - -For production browser applications, contact A/B Smartly support for client-side SDK recommendations. - ## Getting Started Please follow the [installation](#installation) instructions before trying the following code. @@ -306,6 +289,15 @@ context.track("payment", { item_count: 1, total_amount: 1999.99 }); ### Publishing Pending Data +Sometimes it is necessary to ensure all events have been published to the A/B Smartly collector, before proceeding. +One such case is when the user is about to navigate away right before being exposed to a treatment. +You can explicitly call the `publish()` method, which returns a promise, before navigating away. +```javascript +await context.publish().then(() => { + window.location = "https://www.absmartly.com" +}) +``` + #### Finalizing The `finalize()` method will ensure all events have been published to the A/B Smartly collector, like `publish()`, and will also "seal" the context, throwing an error if any method that could generate an event is called. ```javascript @@ -331,15 +323,6 @@ const context = sdk.createContext(request, { }); ``` -### Finalizing - -The `finalize()` method will ensure all events have been published to the A/B Smartly collector, like `publish()`, and will also "seal" the context, throwing an error if any method is called after finalization. - -```javascript -await context.finalize(); -window.location = "https://www.absmartly.com"; -``` - ### Using a Custom Event Logger The A/B Smartly SDK can be instantiated with an event logger used for all contexts. In addition, an event logger can be specified when creating a particular context, in the `createContext` call options. The example below illustrates this with the implementation of the default event logger, used if none is specified. From d096e057d940a9d65558437b3e829444c618c5b8 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 17 Jun 2026 17:50:52 +0100 Subject: [PATCH 10/25] =?UTF-8?q?feat!:=20release=20v2.0.0=20=E2=80=94=20a?= =?UTF-8?q?ddress=20PR=20#50=20review=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING CHANGE: bump to 2.0.0. ready() resolves true (not the Error), unit IDs with astral characters now hash to canonical UTF-8, and audienceMismatch cache invalidation changes for indeterminate audiences. Review feedback from @calthejuggler: - Revert gratuitous input-validation throws on context methods (attribute/peek/treatment/track/variableValue/peekVariableValue/ override/customAssignment/customFieldValue/customFieldValueType and experiments() return); keep only correctness-motivated breaks. - getAttribute(): reverse-scan early-return instead of full-array mutation. - setInterval refresh scheduling collapsed to a one-liner. - sdk.ts: iterate typed clientOptionKeys to drop the double cast; hoist context-option default constants to module scope. - fetch.ts: single typed cast for the global fetch bind. - Reorganize tests: remove fixes.test.js (ReDoS tests stale after revert, EqualsOperator dup'd into eq.test.js) and co-locate the rest into context/sdk/client/abort-controller-shim/fetch-shim test files; rename fixes-constructors.test.js -> constructor-options.test.js, deduped into one describe block. - Rewrite README migration guide for v1 -> v2. npm test 997/997, compile clean, lint clean. --- README.md | 16 +- package-lock.json | 4 +- package.json | 2 +- src/__tests__/abort-controller-shim.test.js | 41 ++ src/__tests__/client.test.js | 15 + ...rs.test.js => constructor-options.test.js} | 63 +-- src/__tests__/context.test.js | 232 ++++++++++ src/__tests__/fetch-shim.test.js | 9 + src/__tests__/fixes.test.js | 436 ------------------ src/__tests__/jsonexpr/operators/eq.test.js | 4 + src/__tests__/sdk.test.js | 17 + src/context.ts | 61 +-- src/fetch.ts | 4 +- src/sdk.ts | 16 +- 14 files changed, 372 insertions(+), 548 deletions(-) rename src/__tests__/{fixes-constructors.test.js => constructor-options.test.js} (51%) delete mode 100644 src/__tests__/fixes.test.js diff --git a/README.md b/README.md index edd7080..e117d2a 100644 --- a/README.md +++ b/README.md @@ -507,9 +507,9 @@ document.getElementById("checkout-btn").addEventListener("click", () => { }); ``` -## Migration Guide +## Migration Guide (v1 → v2) -This version includes minor breaking changes made for cross-SDK consistency and correctness. These align the JavaScript SDK with the Python, Swift, Java, and other A/B Smartly SDKs. +Version 2.0.0 contains breaking changes made for cross-SDK consistency and correctness. These align the JavaScript SDK with the Python, Swift, Java, and other A/B Smartly SDKs. Most applications will not need code changes, but review the items below. ### `ready()` no longer resolves with the Error object on failure @@ -527,9 +527,17 @@ const variant = context.treatment("exp_test"); // returns 0 (control) on failure **When this might be a problem:** If your code used the return value as the Error object (e.g., `const err = await context.ready(); logError(err)`), it will now receive `true` instead. Use `context.readyError()` to access the error instead. -### `override()` now throws after finalization +### Unit IDs containing astral characters now hash to canonical UTF-8 -`override()` now calls `_checkNotFinalized()`, consistent with `customAssignment()`, `track()`, and `attribute()`. Previously, overrides could be set on a finalized context silently with no effect. +**Before:** `stringToUint8Array` (used to hash unit IDs for variant assignment) encoded each UTF-16 code unit independently. A character outside the Basic Multilingual Plane (≥ U+10000, e.g. an emoji) was encoded as an invalid CESU-8 byte sequence rather than canonical UTF-8. + +**After:** Unit IDs are encoded as canonical 4-byte UTF-8, matching the A/B Smartly collector (which hashes with `UTF_8`) and the SDKs already using native UTF-8 (Go, Python, Ruby, etc.). + +**When this might be a problem:** A unit ID that contains an astral character (emoji, rare CJK, etc.) may now be assigned a **different variant** than it was under v1. **Unit IDs composed entirely of BMP characters (≤ U+FFFF) — which covers essentially all typical session IDs, UUIDs, and user IDs — are unaffected.** This only changes assignment for units whose IDs contain astral characters. + +### `audienceMismatch` cache invalidation on indeterminate audiences + +When an audience cannot be evaluated to a boolean (a malformed or non-boolean filter), the cached assignment's `audienceMismatch` flag is now left unchanged instead of being reset to `false`. This keeps a previously valid cached assignment from being needlessly invalidated. Assignment results for well-formed audiences are unchanged. ## About A/B Smartly diff --git a/package-lock.json b/package-lock.json index 50528ba..d921a01 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@absmartly/javascript-sdk", - "version": "1.14.0-beta.1", + "version": "2.0.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@absmartly/javascript-sdk", - "version": "1.14.0-beta.1", + "version": "2.0.0", "license": "Apache-2.0", "dependencies": { "@babel/runtime": "^7.29.2", diff --git a/package.json b/package.json index 220053b..1f074df 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@absmartly/javascript-sdk", - "version": "1.14.0-beta.1", + "version": "2.0.0", "description": "A/B Smartly Javascript SDK", "homepage": "https://github.com/absmartly/javascript-sdk#README.md", "bugs": "https://github.com/absmartly/javascript-sdk/issues", diff --git a/src/__tests__/abort-controller-shim.test.js b/src/__tests__/abort-controller-shim.test.js index fdc3ee0..cc227f5 100644 --- a/src/__tests__/abort-controller-shim.test.js +++ b/src/__tests__/abort-controller-shim.test.js @@ -88,4 +88,45 @@ describe("AbortController", () => { expect(aborter[Symbol.toStringTag]).toEqual("AbortController"); }); + + describe("signal.reason", () => { + it("should set default reason on abort()", () => { + const controller = new AbortController(); + controller.abort(); + expect(controller.signal.aborted).toBe(true); + expect(controller.signal.reason).toBeInstanceOf(Error); + expect(controller.signal.reason.message).toBe("The operation was aborted."); + }); + + it("should set custom reason on abort(reason)", () => { + const controller = new AbortController(); + const customReason = new Error("custom abort"); + controller.abort(customReason); + expect(controller.signal.reason).toBe(customReason); + }); + + it("should have undefined reason before abort", () => { + const controller = new AbortController(); + expect(controller.signal.reason).toBeUndefined(); + }); + }); + + describe("dispatchEvent onabort handling", () => { + it("should call onabort handler on abort dispatch", () => { + const signal = new AbortSignal(); + const handler = jest.fn(); + signal.onabort = handler; + signal.dispatchEvent({ type: "abort" }); + expect(handler).toHaveBeenCalledTimes(1); + expect(handler).toHaveBeenCalledWith({ type: "abort" }); + }); + + it("should not call onabort for non-abort events", () => { + const signal = new AbortSignal(); + const handler = jest.fn(); + signal.onabort = handler; + signal.dispatchEvent({ type: "other" }); + expect(handler).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/__tests__/client.test.js b/src/__tests__/client.test.js index d868fc5..e8738e1 100644 --- a/src/__tests__/client.test.js +++ b/src/__tests__/client.test.js @@ -1189,4 +1189,19 @@ describe("Client", () => { done(); }); }); + + describe("timeout option", () => { + it("should accept an explicit timeout of 0 (nullish coalescing, not falsy)", () => { + const client = new Client({ + endpoint, + agent, + environment, + apiKey, + application, + timeout: 0, + }); + + expect(client).toBeInstanceOf(Client); + }); + }); }); diff --git a/src/__tests__/fixes-constructors.test.js b/src/__tests__/constructor-options.test.js similarity index 51% rename from src/__tests__/fixes-constructors.test.js rename to src/__tests__/constructor-options.test.js index 5a30e56..4e0b437 100644 --- a/src/__tests__/fixes-constructors.test.js +++ b/src/__tests__/constructor-options.test.js @@ -1,14 +1,14 @@ -// Constructor-merge tests for SDK and Client. Kept in a separate file from -// fixes.test.js so the file-scope jest.mock("../client") / jest.mock("../sdk") -// in that file doesn't replace these constructors with Jest doubles, which -// would make these assertions vacuous (passing without exercising the real -// _extractClientOptions / option-merge logic). +// Constructor option-merging tests for SDK and Client. +// +// These deliberately use the real SDK and Client constructors (no jest.mock), +// so they exercise the actual _extractClientOptions / option-merge logic +// rather than Jest doubles, which would make the assertions vacuous. import SDK from "../sdk"; import Client from "../client"; -describe("Fix #11: SDK._extractClientOptions uses includes", () => { - it("should extract client options and pass them to the real Client", () => { +describe("SDK and Client constructor option merging", () => { + it("should extract client options from SDK options and pass them to the real Client", () => { const sdk = new SDK({ agent: "test-agent", apiKey: "key", @@ -24,10 +24,20 @@ describe("Fix #11: SDK._extractClientOptions uses includes", () => { expect(client.getEnvironment()).toBe("test"); expect(client.getApplication()).toEqual({ name: "app", version: 0 }); }); -}); -describe("Fix #33: Client constructor uses spread", () => { - it("should merge defaults with provided options and apply them", () => { + it("should accept the SDK application option as an object", () => { + const sdk = new SDK({ + agent: "test", + apiKey: "key", + application: { name: "myapp", version: "1.2.3" }, + endpoint: "http://localhost", + environment: "prod", + }); + + expect(sdk.getClient().getApplication()).toEqual({ name: "myapp", version: "1.2.3" }); + }); + + it("should merge provided Client options with the defaults", () => { const client = new Client({ endpoint: "http://test", agent: "custom-agent", @@ -42,7 +52,7 @@ describe("Fix #33: Client constructor uses spread", () => { expect(client.getApplication()).toEqual({ name: "myapp", version: 0 }); }); - it("should use defaults when optional fields are omitted", () => { + it("should fall back to the default agent when it is omitted", () => { const client = new Client({ endpoint: "http://test", environment: "prod", @@ -53,34 +63,3 @@ describe("Fix #33: Client constructor uses spread", () => { expect(client.getAgent()).toBe("javascript-client"); }); }); - -describe("Fix #34: SDK._contextOptions uses spread", () => { - it("should merge custom options with defaults via the real constructor", () => { - const sdk = new SDK({ - agent: "test", - apiKey: "key", - application: "app", - endpoint: "http://localhost", - environment: "test", - retries: 2, - timeout: 1234, - }); - - const client = sdk.getClient(); - expect(client).toBeInstanceOf(Client); - expect(client.getAgent()).toBe("test"); - expect(client.getEnvironment()).toBe("test"); - }); - - it("should accept application as an object", () => { - const sdk = new SDK({ - agent: "test", - apiKey: "key", - application: { name: "myapp", version: "1.2.3" }, - endpoint: "http://localhost", - environment: "prod", - }); - - expect(sdk.getClient().getApplication()).toEqual({ name: "myapp", version: "1.2.3" }); - }); -}); diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index b30a472..a6d1a6c 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5132,3 +5132,235 @@ describe("Context", () => { }); }); }); + +describe("Context input handling and lifecycle regressions", () => { + const contextOptions = { + publishDelay: -1, + refreshPeriod: 0, + }; + + const contextParams = { + units: { + session_id: "test-session", + }, + }; + + function newMockSDK() { + const sdk = new SDK(); + const publisher = new ContextPublisher(); + const provider = new ContextDataProvider(); + + sdk.getContextDataProvider.mockReturnValue(provider); + sdk.getContextPublisher.mockReturnValue(publisher); + sdk.getClient.mockReturnValue(new Client()); + sdk.getEventLogger.mockReturnValue(SDK.defaultEventLogger); + + return sdk; + } + + describe("ready() error handling and readyError()", () => { + it("should store error via readyError() when context fetch fails", async () => { + const error = new Error("fetch failed"); + const context = new Context(newMockSDK(), contextOptions, contextParams, Promise.reject(error)); + await context.ready(); + + expect(context.isFailed()).toBe(true); + expect(context.readyError()).toBe(error); + }); + + it("should return null for readyError() when no failure", () => { + const context = new Context(newMockSDK(), contextOptions, contextParams, { experiments: [] }); + expect(context.readyError()).toBe(null); + }); + + it("should allow treatment/peek/track calls after failed init without throwing", async () => { + const error = new Error("fetch failed"); + const context = new Context(newMockSDK(), contextOptions, contextParams, Promise.reject(error)); + const result = await context.ready(); + + expect(result).toBe(true); + expect(context.isFailed()).toBe(true); + expect(context.isReady()).toBe(true); + + expect(context.treatment("any_experiment")).toBe(0); + expect(context.peek("any_experiment")).toBe(0); + expect(context.variableValue("any_key", "fallback")).toBe("fallback"); + expect(context.peekVariableValue("any_key", "fallback")).toBe("fallback"); + expect(context.experiments()).toBeUndefined(); + expect(context.variableKeys()).toEqual({}); + + expect(() => context.track("goal_name")).not.toThrow(); + expect(() => context.attribute("attr", "value")).not.toThrow(); + }); + }); + + describe("variable resolution over experiment arrays", () => { + it("should handle unknown variable keys without error", () => { + const context = new Context( + newMockSDK(), + contextOptions, + { + units: { session_id: "e791e240fcd3df7d238cfc285f475e8152fcc0ec" }, + }, + { + experiments: [ + { + id: 1, + name: "exp_test", + iteration: 1, + unitType: "session_id", + seedHi: 3603515, + seedLo: 233373850, + split: [0.5, 0.5], + trafficSeedHi: 449867249, + trafficSeedLo: 455443629, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + audience: null, + audienceStrict: false, + variants: [ + { name: "A", config: null }, + { name: "B", config: '{"color":"red"}' }, + ], + customFieldValues: null, + }, + ], + } + ); + + expect(context.variableValue("nonexistent_key", "default")).toBe("default"); + }); + }); + + describe("error logging routed through eventLogger", () => { + it("should not call console.error directly for custom field parse errors", () => { + const eventLogger = jest.fn(); + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + const sdk = newMockSDK(); + sdk.getEventLogger.mockReturnValue(eventLogger); + const context = new Context( + sdk, + { publishDelay: -1, refreshPeriod: 0, eventLogger }, + { units: { session_id: "test" } }, + { + experiments: [ + { + id: 1, + name: "exp", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 1, + split: [1], + trafficSeedHi: 1, + trafficSeedLo: 1, + trafficSplit: [0, 1], + fullOnVariant: 0, + audience: null, + audienceStrict: false, + variants: [{ name: "A", config: null }], + customFieldValues: [{ name: "bad_json", value: "{invalid", type: "json" }], + }, + ], + } + ); + + context.customFieldValue("exp", "bad_json"); + expect(errorSpy).not.toHaveBeenCalled(); + expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); + errorSpy.mockRestore(); + }); + + it("should route variant config parse errors through eventLogger", () => { + const eventLogger = jest.fn(); + const sdk = newMockSDK(); + sdk.getEventLogger.mockReturnValue(eventLogger); + + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + const context = new Context( + sdk, + { publishDelay: -1, refreshPeriod: 0, eventLogger }, + { units: { session_id: "test" } }, + { + experiments: [ + { + id: 1, + name: "exp_bad_config", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 1, + split: [1], + trafficSeedHi: 1, + trafficSeedLo: 1, + trafficSplit: [0, 1], + fullOnVariant: 0, + audience: null, + audienceStrict: false, + variants: [{ name: "A", config: "{invalid json}" }], + customFieldValues: null, + }, + ], + } + ); + + expect(errorSpy).not.toHaveBeenCalled(); + expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); + errorSpy.mockRestore(); + }); + }); + + describe("finalizing state", () => { + it("should expose isFinalizing/isFinalized as false before finalize", () => { + const sdk = newMockSDK(); + sdk.getEventLogger.mockReturnValue(jest.fn()); + + const context = new Context( + sdk, + { publishDelay: -1, refreshPeriod: 0 }, + { units: { session_id: "test" } }, + { experiments: [] } + ); + + expect(context.isFinalizing()).toBe(false); + expect(context.isFinalized()).toBe(false); + }); + }); + + describe("attribute map caching", () => { + it("should return correct attributes after multiple attribute() calls", () => { + const sdk = newMockSDK(); + sdk.getEventLogger.mockReturnValue(jest.fn()); + + const context = new Context( + sdk, + { publishDelay: -1, refreshPeriod: 0 }, + { units: { session_id: "test" } }, + { experiments: [] } + ); + + context.attribute("age", 25); + context.attribute("country", "US"); + + const attrs = context.getAttributes(); + expect(attrs).toEqual({ age: 25, country: "US" }); + }); + }); + + describe("getOptions() returns a shallow copy", () => { + it("should not allow mutation of internal options", () => { + const sdk = newMockSDK(); + sdk.getEventLogger.mockReturnValue(jest.fn()); + + const originalOptions = { publishDelay: 100, refreshPeriod: 0 }; + const context = new Context(sdk, originalOptions, { units: { session_id: "test" } }, { experiments: [] }); + + const opts = context.getOptions(); + opts.publishDelay = 9999; + + expect(context.getOptions().publishDelay).toBe(100); + }); + }); +}); diff --git a/src/__tests__/fetch-shim.test.js b/src/__tests__/fetch-shim.test.js index d214b7a..542d1a2 100644 --- a/src/__tests__/fetch-shim.test.js +++ b/src/__tests__/fetch-shim.test.js @@ -133,3 +133,12 @@ describe("fetch", () => { }); }); }); + +describe("fetch implementation resolver", () => { + it("should resolve to a function, never undefined", async () => { + const fetchModule = await import("../fetch"); + const fetchImpl = fetchModule.default; + expect(fetchImpl).not.toBeUndefined(); + expect(typeof fetchImpl).toBe("function"); + }); +}); diff --git a/src/__tests__/fixes.test.js b/src/__tests__/fixes.test.js deleted file mode 100644 index f18c77e..0000000 --- a/src/__tests__/fixes.test.js +++ /dev/null @@ -1,436 +0,0 @@ -import { MatchOperator } from "../jsonexpr/operators/match"; -import { EqualsOperator } from "../jsonexpr/operators/eq"; -import { mockEvaluator } from "./jsonexpr/operators/evaluator"; -import { AbortController as ShimAbortController, AbortSignal as ShimAbortSignal } from "../abort-controller-shim"; -import { AudienceMatcher } from "../matcher"; -import SDK from "../sdk"; -import Context from "../context"; -import Client from "../client"; -import { ContextPublisher } from "../publisher"; -import { ContextDataProvider } from "../provider"; - -jest.mock("../client"); -jest.mock("../sdk"); -jest.mock("../provider"); -jest.mock("../publisher"); - -describe("Fix #1: ReDoS protection in MatchOperator", () => { - const operator = new MatchOperator(); - const evaluator = mockEvaluator(); - - it("should reject patterns with nested quantifiers", () => { - expect(operator.evaluate(evaluator, ["aaaaaaaaaa", "(a+)+$"])).toBe(null); - expect(operator.evaluate(evaluator, ["test", "(a*)*b"])).toBe(null); - expect(operator.evaluate(evaluator, ["test", "(x+){2}"])).toBe(null); - }); - - it("should still allow safe patterns", () => { - expect(operator.evaluate(evaluator, ["abc", "a+"])).toBe(true); - expect(operator.evaluate(evaluator, ["abc", "^abc$"])).toBe(true); - expect(operator.evaluate(evaluator, ["abc", "[a-z]+"])).toBe(true); - }); - - it("should reject patterns exceeding max length", () => { - const longPattern = "a".repeat(1001); - expect(operator.evaluate(evaluator, ["test", longPattern])).toBe(null); - }); - - it("should reject text exceeding max length", () => { - const longText = "a".repeat(10001); - expect(operator.evaluate(evaluator, [longText, "a"])).toBe(null); - }); - - it("should cache compiled regexes", () => { - expect(operator.evaluate(evaluator, ["abc", "abc"])).toBe(true); - expect(operator.evaluate(evaluator, ["abc", "abc"])).toBe(true); - }); - - it("should return null for invalid regex", () => { - expect(operator.evaluate(evaluator, ["test", "[invalid"])).toBe(null); - }); - - it("should not use console.error", () => { - const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); - operator.evaluate(evaluator, ["test", "[invalid"]); - operator.evaluate(evaluator, ["test", "a".repeat(1001)]); - operator.evaluate(evaluator, ["a".repeat(10001), "a"]); - expect(errorSpy).not.toHaveBeenCalled(); - errorSpy.mockRestore(); - }); -}); - -describe("Fix #2: ready() error handling and readyError()", () => { - const sdk = new SDK(); - const publisher = new ContextPublisher(); - const provider = new ContextDataProvider(); - - sdk.getContextDataProvider.mockReturnValue(provider); - sdk.getContextPublisher.mockReturnValue(publisher); - sdk.getClient.mockReturnValue(new Client()); - sdk.getEventLogger.mockReturnValue(SDK.defaultEventLogger); - - const contextOptions = { - publishDelay: -1, - refreshPeriod: 0, - }; - - const contextParams = { - units: { - session_id: "test-session", - }, - }; - - it("should store error via readyError() when context fetch fails", async () => { - const error = new Error("fetch failed"); - const context = new Context(sdk, contextOptions, contextParams, Promise.reject(error)); - await context.ready(); - - expect(context.isFailed()).toBe(true); - expect(context.readyError()).toBe(error); - }); - - it("should return null for readyError() when no failure", () => { - const context = new Context(sdk, contextOptions, contextParams, { experiments: [] }); - expect(context.readyError()).toBe(null); - }); - - it("should allow treatment/peek/track calls after failed init without throwing", async () => { - const error = new Error("fetch failed"); - const context = new Context(sdk, contextOptions, contextParams, Promise.reject(error)); - const result = await context.ready(); - - expect(result).toBe(true); - expect(context.isFailed()).toBe(true); - expect(context.isReady()).toBe(true); - - expect(context.treatment("any_experiment")).toBe(0); - expect(context.peek("any_experiment")).toBe(0); - expect(context.variableValue("any_key", "fallback")).toBe("fallback"); - expect(context.peekVariableValue("any_key", "fallback")).toBe("fallback"); - expect(context.experiments()).toEqual([]); - expect(context.variableKeys()).toEqual({}); - - expect(() => context.track("goal_name")).not.toThrow(); - expect(() => context.attribute("attr", "value")).not.toThrow(); - }); -}); - -describe("Fix #3: for...of on array in _resolveVariableValue", () => { - const sdk = new SDK(); - const publisher = new ContextPublisher(); - const provider = new ContextDataProvider(); - - sdk.getContextDataProvider.mockReturnValue(provider); - sdk.getContextPublisher.mockReturnValue(publisher); - sdk.getClient.mockReturnValue(new Client()); - sdk.getEventLogger.mockReturnValue(SDK.defaultEventLogger); - - const contextOptions = { - publishDelay: -1, - refreshPeriod: 0, - }; - - const contextParams = { - units: { - session_id: "e791e240fcd3df7d238cfc285f475e8152fcc0ec", - }, - }; - - it("should handle unknown variable keys without error", () => { - const context = new Context(sdk, contextOptions, contextParams, { - experiments: [ - { - id: 1, - name: "exp_test", - iteration: 1, - unitType: "session_id", - seedHi: 3603515, - seedLo: 233373850, - split: [0.5, 0.5], - trafficSeedHi: 449867249, - trafficSeedLo: 455443629, - trafficSplit: [0.0, 1.0], - fullOnVariant: 0, - audience: null, - audienceStrict: false, - variants: [ - { name: "A", config: null }, - { name: "B", config: '{"color":"red"}' }, - ], - customFieldValues: null, - }, - ], - }); - - expect(context.variableValue("nonexistent_key", "default")).toBe("default"); - }); -}); - -describe("Fix #4: console.error routed through eventLogger", () => { - const sdk = new SDK(); - const publisher = new ContextPublisher(); - const provider = new ContextDataProvider(); - - sdk.getContextDataProvider.mockReturnValue(provider); - sdk.getContextPublisher.mockReturnValue(publisher); - sdk.getClient.mockReturnValue(new Client()); - - const contextParams = { - units: { - session_id: "test", - }, - }; - - it("should not call console.error directly for custom field parse errors", () => { - const eventLogger = jest.fn(); - const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); - - sdk.getEventLogger.mockReturnValue(eventLogger); - const context = new Context(sdk, { publishDelay: -1, refreshPeriod: 0, eventLogger }, contextParams, { - experiments: [ - { - id: 1, - name: "exp", - iteration: 1, - unitType: "session_id", - seedHi: 1, - seedLo: 1, - split: [1], - trafficSeedHi: 1, - trafficSeedLo: 1, - trafficSplit: [0, 1], - fullOnVariant: 0, - audience: null, - audienceStrict: false, - variants: [{ name: "A", config: null }], - customFieldValues: [{ name: "bad_json", value: "{invalid", type: "json" }], - }, - ], - }); - - context.customFieldValue("exp", "bad_json"); - expect(errorSpy).not.toHaveBeenCalled(); - expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); - errorSpy.mockRestore(); - }); - - it("should not call console.error in AudienceMatcher on parse failure", () => { - const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); - const matcher = new AudienceMatcher(); - matcher.evaluate("{invalid json", {}); - expect(errorSpy).not.toHaveBeenCalled(); - errorSpy.mockRestore(); - }); - - it("should route variant config parse errors through eventLogger", () => { - const eventLogger = jest.fn(); - sdk.getEventLogger.mockReturnValue(eventLogger); - - const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); - - const context = new Context(sdk, { publishDelay: -1, refreshPeriod: 0, eventLogger }, contextParams, { - experiments: [ - { - id: 1, - name: "exp_bad_config", - iteration: 1, - unitType: "session_id", - seedHi: 1, - seedLo: 1, - split: [1], - trafficSeedHi: 1, - trafficSeedLo: 1, - trafficSplit: [0, 1], - fullOnVariant: 0, - audience: null, - audienceStrict: false, - variants: [{ name: "A", config: "{invalid json}" }], - customFieldValues: null, - }, - ], - }); - - expect(errorSpy).not.toHaveBeenCalled(); - expect(eventLogger).toHaveBeenCalledWith(context, "error", expect.any(Error)); - errorSpy.mockRestore(); - }); -}); - -describe("Fix #5: _finalizing type cleanup", () => { - it("should not use boolean for _finalizing", () => { - const sdk = new SDK(); - const publisher = new ContextPublisher(); - const provider = new ContextDataProvider(); - - sdk.getContextDataProvider.mockReturnValue(provider); - sdk.getContextPublisher.mockReturnValue(publisher); - sdk.getClient.mockReturnValue(new Client()); - sdk.getEventLogger.mockReturnValue(jest.fn()); - - const context = new Context( - sdk, - { publishDelay: -1, refreshPeriod: 0 }, - { units: { session_id: "test" } }, - { experiments: [] } - ); - - expect(context.isFinalizing()).toBe(false); - expect(context.isFinalized()).toBe(false); - }); -}); - -// Fix #11, #33, #34: real-constructor tests live in fixes-constructors.test.js -// (file-scope jest.mock("../client") and jest.mock("../sdk") in this file -// would otherwise make those tests vacuous against Jest doubles). - -describe("Fix #12: SDK.defaultEventLogger logs error message", () => { - let ActualSDK; - beforeAll(() => { - ActualSDK = jest.requireActual("../sdk").default; - }); - - it("should log full Error object to preserve stack traces", () => { - const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); - const error = new Error("something failed"); - ActualSDK.defaultEventLogger(null, "error", error); - expect(errorSpy).toHaveBeenCalledWith(error); - errorSpy.mockRestore(); - }); - - it("should log raw data for non-Error values", () => { - const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); - ActualSDK.defaultEventLogger(null, "error", "plain text error"); - expect(errorSpy).toHaveBeenCalledWith("plain text error"); - errorSpy.mockRestore(); - }); -}); - -describe("Fix #13: _getAttributesMap caching", () => { - const sdk = new SDK(); - const publisher = new ContextPublisher(); - const provider = new ContextDataProvider(); - - sdk.getContextDataProvider.mockReturnValue(provider); - sdk.getContextPublisher.mockReturnValue(publisher); - sdk.getClient.mockReturnValue(new Client()); - sdk.getEventLogger.mockReturnValue(jest.fn()); - - it("should return correct attributes after multiple attribute() calls", () => { - const context = new Context( - sdk, - { publishDelay: -1, refreshPeriod: 0 }, - { units: { session_id: "test" } }, - { experiments: [] } - ); - - context.attribute("age", 25); - context.attribute("country", "US"); - - const attrs = context.getAttributes(); - expect(attrs).toEqual({ age: 25, country: "US" }); - }); -}); - -describe("Fix #15: fetch.ts throws on missing implementation", () => { - it("should not export undefined", async () => { - const fetchModule = await import("../fetch"); - const fetchImpl = fetchModule.default; - expect(fetchImpl).not.toBeUndefined(); - expect(typeof fetchImpl).toBe("function"); - }); -}); - -describe("Fix #16: Client.request timeout uses nullish coalescing", () => { - it("should accept timeout of 0 in options", () => { - const client = new Client({ - endpoint: "http://test", - agent: "test", - environment: "test", - apiKey: "key", - application: "app", - timeout: 5000, - }); - - expect(client).toBeInstanceOf(Client); - }); -}); - -describe("Fix #21: AbortController shim sets signal.reason", () => { - it("should set default reason on abort()", () => { - const controller = new ShimAbortController(); - controller.abort(); - expect(controller.signal.aborted).toBe(true); - expect(controller.signal.reason).toBeInstanceOf(Error); - expect(controller.signal.reason.message).toBe("The operation was aborted."); - }); - - it("should set custom reason on abort(reason)", () => { - const controller = new ShimAbortController(); - const customReason = new Error("custom abort"); - controller.abort(customReason); - expect(controller.signal.reason).toBe(customReason); - }); - - it("should have undefined reason before abort", () => { - const controller = new ShimAbortController(); - expect(controller.signal.reason).toBeUndefined(); - }); -}); - -describe("Fix #25: Context.getOptions() returns shallow copy", () => { - it("should not allow mutation of internal options", () => { - const sdk = new SDK(); - const publisher = new ContextPublisher(); - const provider = new ContextDataProvider(); - - sdk.getContextDataProvider.mockReturnValue(provider); - sdk.getContextPublisher.mockReturnValue(publisher); - sdk.getClient.mockReturnValue(new Client()); - sdk.getEventLogger.mockReturnValue(jest.fn()); - - const originalOptions = { publishDelay: 100, refreshPeriod: 0 }; - const context = new Context(sdk, originalOptions, { units: { session_id: "test" } }, { experiments: [] }); - - const opts = context.getOptions(); - opts.publishDelay = 9999; - - expect(context.getOptions().publishDelay).toBe(100); - }); -}); - -describe("Fix #32: AbortSignal dispatchEvent uses explicit onabort", () => { - it("should call onabort handler on dispatch", () => { - const signal = new ShimAbortSignal(); - const handler = jest.fn(); - signal.onabort = handler; - signal.dispatchEvent({ type: "abort" }); - expect(handler).toHaveBeenCalledTimes(1); - expect(handler).toHaveBeenCalledWith({ type: "abort" }); - }); - - it("should not call onabort for non-abort events", () => { - const signal = new ShimAbortSignal(); - const handler = jest.fn(); - signal.onabort = handler; - signal.dispatchEvent({ type: "other" }); - expect(handler).not.toHaveBeenCalled(); - }); -}); - -describe("Fix #20: EqualsOperator without redundant Array.isArray", () => { - const operator = new EqualsOperator(); - const evaluator = mockEvaluator(); - - it("should evaluate equality correctly", () => { - expect(operator.evaluate(evaluator, [1, 1])).toBe(true); - expect(operator.evaluate(evaluator, [1, 2])).toBe(false); - }); - - it("should handle null comparison", () => { - expect(operator.evaluate(evaluator, [null, null])).toBe(null); - }); - - it("should handle empty args", () => { - expect(operator.evaluate(evaluator, [])).toBe(null); - }); -}); diff --git a/src/__tests__/jsonexpr/operators/eq.test.js b/src/__tests__/jsonexpr/operators/eq.test.js index d7132af..40c9803 100644 --- a/src/__tests__/jsonexpr/operators/eq.test.js +++ b/src/__tests__/jsonexpr/operators/eq.test.js @@ -106,5 +106,9 @@ describe("EqOperator", () => { evaluator.evaluate.mockClear(); evaluator.compare.mockClear(); }); + + it("should return null for empty args", () => { + expect(operator.evaluate(evaluator, [])).toBe(null); + }); }); }); diff --git a/src/__tests__/sdk.test.js b/src/__tests__/sdk.test.js index 3dc75de..7a0ad73 100644 --- a/src/__tests__/sdk.test.js +++ b/src/__tests__/sdk.test.js @@ -476,4 +476,21 @@ describe("SDK", () => { done(); }); }); + + describe("defaultEventLogger", () => { + it("should log full Error object to preserve stack traces", () => { + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + const error = new Error("something failed"); + SDK.defaultEventLogger(null, "error", error); + expect(errorSpy).toHaveBeenCalledWith(error); + errorSpy.mockRestore(); + }); + + it("should log raw data for non-Error values", () => { + const errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + SDK.defaultEventLogger(null, "error", "plain text error"); + expect(errorSpy).toHaveBeenCalledWith("plain text error"); + errorSpy.mockRestore(); + }); + }); }); diff --git a/src/context.ts b/src/context.ts index 95c9c07..fb09ad8 100644 --- a/src/context.ts +++ b/src/context.ts @@ -331,17 +331,13 @@ export default class Context { } getAttribute(attrName: string) { - let result; - for (const attr of this._attrs) { - if (attr.name === attrName) result = attr.value; + for (let i = this._attrs.length - 1; i >= 0; i--) { + if (this._attrs[i].name === attrName) return this._attrs[i].value; } - return result; + return undefined; } attribute(attrName: string, value: unknown) { - if (typeof attrName !== "string" || attrName.trim().length === 0) { - throw new Error("Attribute name must be a non-empty string"); - } this._checkNotFinalized(); this._attrs.push({ name: attrName, value: value, setAt: Date.now() }); @@ -363,30 +359,21 @@ export default class Context { } peek(experimentName: string) { - if (typeof experimentName !== "string" || experimentName.trim().length === 0) { - throw new Error("Experiment name must be a non-empty string"); - } this._checkReady(true); return this._peek(experimentName).variant; } treatment(experimentName: string) { - if (typeof experimentName !== "string" || experimentName.trim().length === 0) { - throw new Error("Experiment name must be a non-empty string"); - } this._checkReady(true); return this._treatment(experimentName).variant; } - track(goalName: string, properties?: Record | null) { - if (typeof goalName !== "string" || goalName.trim().length === 0) { - throw new Error("Goal name must be a non-empty string"); - } + track(goalName: string, properties?: Record) { this._checkNotFinalized(); - return this._track(goalName, properties ?? undefined); + return this._track(goalName, properties); } finalize(requestOptions?: ClientRequestOptions) { @@ -396,22 +383,16 @@ export default class Context { experiments() { this._checkReady(); - return this._data.experiments?.map((x) => x.name) ?? []; + return this._data.experiments?.map((x) => x.name); } variableValue(key: string, defaultValue: string): string { - if (typeof key !== "string" || key.trim().length === 0) { - throw new Error("Variable key must be a non-empty string"); - } this._checkReady(true); return this._variableValue(key, defaultValue); } peekVariableValue(key: string, defaultValue: string): string { - if (typeof key !== "string" || key.trim().length === 0) { - throw new Error("Variable key must be a non-empty string"); - } this._checkReady(true); return this._peekVariable(key, defaultValue); @@ -433,12 +414,6 @@ export default class Context { } override(experimentName: string, variant: number) { - if (typeof experimentName !== "string" || experimentName.trim().length === 0) { - throw new Error("Experiment name must be a non-empty string"); - } - if (typeof variant !== "number" || variant < 0 || !Number.isInteger(variant)) { - throw new Error("Variant must be a non-negative integer"); - } // override() is allowed after finalize() (parity with the production SDK, // which sets overrides unconditionally). this._overrides = Object.assign(this._overrides, { [experimentName]: variant }); @@ -451,12 +426,6 @@ export default class Context { } customAssignment(experimentName: string, variant: number) { - if (typeof experimentName !== "string" || experimentName.trim().length === 0) { - throw new Error("Experiment name must be a non-empty string"); - } - if (typeof variant !== "number" || variant < 0 || !Number.isInteger(variant)) { - throw new Error("Variant must be a non-negative integer"); - } this._checkNotFinalized(); this._cassignments[experimentName] = variant; @@ -845,12 +814,6 @@ export default class Context { } customFieldValue(experimentName: string, key: string) { - if (typeof experimentName !== "string" || experimentName.trim().length === 0) { - throw new Error("Experiment name must be a non-empty string"); - } - if (typeof key !== "string" || key.trim().length === 0) { - throw new Error("Field key must be a non-empty string"); - } this._checkReady(true); return this._customFieldValue(experimentName, key); @@ -870,12 +833,6 @@ export default class Context { } customFieldValueType(experimentName: string, key: string) { - if (typeof experimentName !== "string" || experimentName.trim().length === 0) { - throw new Error("Experiment name must be a non-empty string"); - } - if (typeof key !== "string" || key.trim().length === 0) { - throw new Error("Field key must be a non-empty string"); - } this._checkReady(true); return this._customFieldValueType(experimentName, key); @@ -1190,10 +1147,8 @@ export default class Context { this._assignments = assignments; if (!this._failed && this._opts.refreshPeriod > 0 && !this._refreshInterval) { - this._refreshInterval = setInterval(() => { - // _refresh already logs refresh errors via the callback. - this._refresh(); - }, this._opts.refreshPeriod); + // _refresh already logs refresh errors via the callback. + this._refreshInterval = setInterval(() => this._refresh(), this._opts.refreshPeriod); } } diff --git a/src/fetch.ts b/src/fetch.ts index 149277f..018858d 100644 --- a/src/fetch.ts +++ b/src/fetch.ts @@ -19,8 +19,8 @@ function getFetchImplementation() { const globalObj = typeof globalThis !== "undefined" ? globalThis : typeof global !== "undefined" ? global : undefined; if (globalObj !== undefined) { - if ((globalObj as Record).fetch) { - return ((globalObj as Record).fetch as (...args: unknown[]) => unknown).bind(globalObj); + if ((globalObj as { fetch?: typeof fetch }).fetch) { + return (globalObj as { fetch: typeof fetch }).fetch.bind(globalObj); } return function (url: string, opts: Record) { return new Promise((resolve, reject) => { diff --git a/src/sdk.ts b/src/sdk.ts index 2f97ea7..33f7120 100644 --- a/src/sdk.ts +++ b/src/sdk.ts @@ -12,6 +12,10 @@ import { isLongLivedApp } from "./utils"; export type EventLoggerData = Error | Exposure | Goal | ContextData | PublishParams; +const DEFAULT_PUBLISH_DELAY_MS = 100; +const NO_PUBLISH_DELAY = -1; +const NO_REFRESH = 0; + export type EventName = "error" | "ready" | "refresh" | "publish" | "exposure" | "goal" | "finalize"; export type EventLogger = (context: Context, eventName: EventName, data?: EventLoggerData) => void; @@ -45,7 +49,7 @@ export default class SDK { } private static _extractClientOptions(options: ClientOptions & SDKOptions): ClientOptions { - const clientOptionKeys = [ + const clientOptionKeys: (keyof ClientOptions)[] = [ "application", "agent", "apiKey", @@ -59,9 +63,9 @@ export default class SDK { agent: "absmartly-javascript-sdk", }; - for (const [key, value] of Object.entries(options || {})) { - if (clientOptionKeys.includes(key)) { - (extracted as Record)[key] = value; + for (const key of clientOptionKeys) { + if (options?.[key] !== undefined) { + extracted[key] = options[key] as never; } } @@ -125,10 +129,6 @@ export default class SDK { } private static _contextOptions(options?: Partial): ContextOptions { - const DEFAULT_PUBLISH_DELAY_MS = 100; - const NO_PUBLISH_DELAY = -1; - const NO_REFRESH = 0; - return { publishDelay: isLongLivedApp() ? DEFAULT_PUBLISH_DELAY_MS : NO_PUBLISH_DELAY, refreshPeriod: NO_REFRESH, From 2f83082f2ff1eb54f991f568d3790236f03d0837 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 17 Jun 2026 18:04:22 +0100 Subject: [PATCH 11/25] test: use rest-destructure to drop agent in client option test Applies @calthejuggler's suggestion on PR #50 (cleaner than copy-then-delete). --- src/__tests__/client.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/__tests__/client.test.js b/src/__tests__/client.test.js index e8738e1..d9e8e54 100644 --- a/src/__tests__/client.test.js +++ b/src/__tests__/client.test.js @@ -1121,8 +1121,7 @@ describe("Client", () => { }); it("getAgent() should return default agent when not specified", () => { - const optionsWithoutAgent = { ...clientOptions }; - delete optionsWithoutAgent.agent; + const { agent: _, ...optionsWithoutAgent } = clientOptions; const client = new Client(optionsWithoutAgent); expect(client.getAgent()).toEqual("javascript-client"); }); From 7f5d1916483aa49d8394145e23832f2bdbaaf266 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 24 Jun 2026 10:49:13 +0100 Subject: [PATCH 12/25] fix: ignore js/ and types/ build output in prettier format:check ran 'prettier --check' over the generated js/ and types/ dirs (build output, git-ignored but not prettier-ignored), failing the build on stale artifacts. Add them to .prettierignore alongside es/lib/dist. --- .prettierignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.prettierignore b/.prettierignore index 22b4251..92641fe 100644 --- a/.prettierignore +++ b/.prettierignore @@ -2,5 +2,7 @@ node_modules coverage dist es +js lib +types package-lock.json From 70053814930e7d2bbbbe8bc441571abc01c6c697 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 25 Jun 2026 19:11:26 +0100 Subject: [PATCH 13/25] test: add hermetic local-HTTP integration test for real fetch/publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Node http server + public SDK exercises the real client doing GET /context (createContext→ready) and PUT /context (publish), asserting the wire contract (JS sends auth headers on GET too). CI-runnable complement to the live e2e. --- .../local-server-integration.test.js | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 src/__tests__/local-server-integration.test.js diff --git a/src/__tests__/local-server-integration.test.js b/src/__tests__/local-server-integration.test.js new file mode 100644 index 0000000..34f8fc8 --- /dev/null +++ b/src/__tests__/local-server-integration.test.js @@ -0,0 +1,117 @@ +import http from "http"; +import SDK from "../sdk"; + +// Hermetic integration test: spins up a real local HTTP server on an ephemeral +// port, points the SDK's client endpoint at it, and drives the PUBLIC SDK API so +// the REAL HTTP client performs a GET /context (createContext -> ready) and a +// PUT /context (treatment + track -> publish). Asserts the wire contract. +describe("Local server integration (real HTTP)", () => { + let server; + let baseUrl; + const requests = []; + + beforeAll((done) => { + server = http.createServer((req, res) => { + const chunks = []; + req.on("data", (c) => chunks.push(c)); + req.on("end", () => { + const bodyStr = Buffer.concat(chunks).toString("utf8"); + const record = { + method: req.method, + url: req.url, + headers: req.headers, + body: bodyStr.length > 0 ? JSON.parse(bodyStr) : undefined, + }; + requests.push(record); + + res.setHeader("Content-Type", "application/json"); + if (req.method === "GET") { + res.statusCode = 200; + res.end(JSON.stringify({ experiments: [] })); + } else if (req.method === "PUT") { + res.statusCode = 200; + res.end(JSON.stringify({})); + } else { + res.statusCode = 405; + res.end("{}"); + } + }); + }); + + server.listen(0, "127.0.0.1", () => { + const { port } = server.address(); + baseUrl = `http://127.0.0.1:${port}`; + done(); + }); + }); + + afterAll((done) => { + server.close(done); + }); + + it("performs a real GET /context and PUT /context against a local server", async () => { + const sdk = new SDK({ + endpoint: baseUrl, + apiKey: "test-api-key", + application: "www", + environment: "development", + }); + + const context = sdk.createContext( + { + units: { + session_id: "e791e240fcd3df7d238cfc285f475e8152fcc0ec", + user_id: "123456789", + }, + }, + { publishDelay: -1, refreshPeriod: 0 } + ); + + await context.ready(); + + // --- assert the GET /context --- + const getReq = requests.find((r) => r.method === "GET"); + expect(getReq).toBeDefined(); + const getUrl = new URL(getReq.url, baseUrl); + expect(getUrl.pathname).toBe("/context"); + expect(getUrl.searchParams.get("application")).toBe("www"); + expect(getUrl.searchParams.get("environment")).toBe("development"); + // JS sends the full auth header set on GET too (per wire contract). + expect(getReq.headers["x-api-key"]).toBe("test-api-key"); + + // --- drive an exposure + a goal, then publish --- + context.treatment("not_found_experiment"); + context.track("payment", { value: 99 }); + + await context.publish(); + + const putReq = requests.find((r) => r.method === "PUT"); + expect(putReq).toBeDefined(); + const putUrl = new URL(putReq.url, baseUrl); + expect(putUrl.pathname).toBe("/context"); + expect(putUrl.search).toBe(""); + + // --- headers --- + expect(putReq.headers["x-api-key"]).toBe("test-api-key"); + expect(putReq.headers["x-application"]).toBe("www"); + expect(putReq.headers["x-environment"]).toBe("development"); + expect(putReq.headers["x-application-version"]).toBe("0"); + expect(putReq.headers["x-agent"]).toBeDefined(); + expect(putReq.headers["x-agent"].length).toBeGreaterThan(0); + expect(putReq.headers["content-type"]).toMatch(/application\/json/); + + // --- body --- + const body = putReq.body; + expect(body.hashed).toBe(true); + expect(Array.isArray(body.units)).toBe(true); + expect(body.units.length).toBeGreaterThan(0); + expect(body.units[0]).toHaveProperty("type"); + expect(body.units[0]).toHaveProperty("uid"); + expect(typeof body.publishedAt).toBe("number"); + expect(Array.isArray(body.goals)).toBe(true); + expect(body.goals.length).toBeGreaterThan(0); + expect(body.goals[0].name).toBe("payment"); + expect(Array.isArray(body.exposures)).toBe(true); + expect(body.exposures.length).toBeGreaterThan(0); + }); +}); From 277885662ae9da0254f13e9c2b62e7ee2ff308dc Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 11:33:55 +0100 Subject: [PATCH 14/25] fix: address second-round P2 review feedback (context race, client timeout/query, entrypoint parity, docs) - utils.ts: emit U+FFFD for unmatched UTF-16 surrogates in the manual UTF-8 fallback, matching TextEncoder/canonical UTF-8 so hashUnit no longer differs across environments with/without TextEncoder. - context.ts: track the in-flight flush with a shared promise so a concurrent finalize() waits for it instead of racing the synchronous queue reset; restore the snapshot on both synchronous and asynchronous publisher failures; reschedule the automatic publish timer after a restored failed flush. - client.ts: replace URLSearchParams (unavailable under the declared IE 10 browser target) with a manual query-string encoder; stop treating timeout=0 ("no deadline") as an immediate deadline in the retry loop so the retries budget is honored independently of elapsed time. - browser.ts: add the ABsmartly alias to the UMD default export to match the CJS/ES entry point; add an entrypoints test guarding parity between the two. - README.md: fix the refresh example to use the real refreshPeriod option instead of the non-existent refreshInterval. Co-authored-by: jervasion-absmartly --- README.md | 4 +- src/__tests__/client.test.js | 22 +++ src/__tests__/context.test.js | 130 ++++++++++++++ src/__tests__/entrypoints.test.js | 19 ++ src/__tests__/utils.test.js | 68 +++++++ src/browser.ts | 2 +- src/client.ts | 19 +- src/context.ts | 285 ++++++++++++++++++------------ src/utils.ts | 10 +- 9 files changed, 430 insertions(+), 129 deletions(-) create mode 100644 src/__tests__/entrypoints.test.js diff --git a/README.md b/README.md index e117d2a..23133ca 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ When doing full-stack experimentation with A/B Smartly, we recommend creating a ### Refreshing the Context with Fresh Experiment Data -For long-running single-page-applications (SPA), the context is usually created once when the application is first reached. However, any experiments being tracked in your production code, but started after the context was created, will not be triggered. To mitigate this, we can use the `refreshInterval` option when creating the context. +For long-running single-page-applications (SPA), the context is usually created once when the application is first reached. However, any experiments being tracked in your production code, but started after the context was created, will not be triggered. To mitigate this, we can use the `refreshPeriod` option when creating the context. ```javascript const request = { @@ -146,7 +146,7 @@ const request = { }; const context = sdk.createContext(request, { - refreshInterval: 5 * 60 * 1000, // 5 minutes + refreshPeriod: 5 * 60 * 1000, // 5 minutes }); ``` diff --git a/src/__tests__/client.test.js b/src/__tests__/client.test.js index d9e8e54..d029986 100644 --- a/src/__tests__/client.test.js +++ b/src/__tests__/client.test.js @@ -1202,5 +1202,27 @@ describe("Client", () => { expect(client).toBeInstanceOf(Client); }); + + it("should still retry a failing-then-succeeding request when timeout is 0 (no deadline)", (done) => { + fetch + .mockResolvedValueOnce(responseMock(500, "server error", "server error text")) + .mockResolvedValueOnce(responseMock(200, "OK", defaultMockResponse)); + + const client = new Client(Object.assign({}, clientOptions, { timeout: 0, retries: 5 })); + + client + .request({ + method: "GET", + path: "/context", + }) + .then((response) => { + expect(fetch).toHaveBeenCalledTimes(2); + expect(response).toEqual(defaultMockResponse); + + done(); + }); + + advanceFakeTimers(); + }); }); }); diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index a6d1a6c..2b3867b 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4436,6 +4436,136 @@ describe("Context", () => { done(); }); }); + + it("should not finalize while a concurrent publish() is still in flight", (done) => { + jest.useRealTimers(); + + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0 }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + let resolvePublish; + publisher.publish.mockReturnValue( + new Promise((resolve) => { + resolvePublish = resolve; + }) + ); + + const publishPromise = context.publish(); + + // publish() has already reset the internal queue synchronously, so a + // naive pending()-based check would see an empty queue here even though + // the request has not settled yet. + expect(context.pending()).toEqual(0); + + const finalizePromise = context.finalize(); + + // finalize() must not complete (or mark the context finalized) while the + // in-flight publish it is racing against hasn't settled. + expect(context.isFinalized()).toEqual(false); + expect(context.isFinalizing()).toEqual(true); + + // Give any (incorrect) synchronous finalize path a chance to run before + // resolving the in-flight publish. + Promise.resolve() + .then(() => Promise.resolve()) + .then(() => Promise.resolve()) + .then(() => { + expect(context.isFinalized()).toEqual(false); + expect(publisher.publish).toHaveBeenCalledTimes(1); + + resolvePublish(); + + return Promise.all([publishPromise, finalizePromise]); + }) + .then(() => { + expect(context.isFinalized()).toEqual(true); + expect(context.isFinalizing()).toEqual(false); + expect(context.pending()).toEqual(0); + // finalize() waited on the same in-flight publish instead of + // triggering a second, redundant request. + expect(publisher.publish).toHaveBeenCalledTimes(1); + + done(); + }); + }); + + it("should restore the queue and reject when the publisher throws synchronously", (done) => { + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0 }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + const syncError = new Error("synchronous publisher failure"); + publisher.publish.mockImplementation(() => { + throw syncError; + }); + + context.publish().catch((e) => { + expect(e).toBe(syncError); + // The snapshot taken before the (synchronously throwing) publish call + // must be restored so the events are retried on the next flush. + expect(context.pending()).toEqual(1); + + done(); + }); + }); + + it("should reschedule the automatic publish timer after a scheduled flush fails", (done) => { + jest.useFakeTimers("legacy"); + jest.spyOn(global, "setTimeout"); + + const publishDelay = 100; + const context = new Context( + sdk, + { ...contextOptions, publishDelay, refreshPeriod: 0 }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + expect(setTimeout).toHaveBeenCalledTimes(1); + + publisher.publish.mockReturnValueOnce(Promise.reject(new Error("network error"))); + + jest.advanceTimersByTime(publishDelay); + + // Flush the microtask queue so the rejection handler (which restores the + // queue and reschedules) has run before we assert on it. + Promise.resolve() + .then(() => Promise.resolve()) + .then(() => { + expect(context.pending()).toEqual(1); + // A new automatic-publish timer must have been scheduled for the + // restored batch, otherwise it is silently dropped forever. + expect(setTimeout).toHaveBeenCalledTimes(2); + + publisher.publish.mockReturnValueOnce(Promise.resolve()); + + jest.advanceTimersByTime(publishDelay); + + Promise.resolve() + .then(() => Promise.resolve()) + .then(() => { + expect(context.pending()).toEqual(0); + expect(publisher.publish).toHaveBeenCalledTimes(2); + + done(); + }); + }); + }); }); describe("override()", () => { diff --git a/src/__tests__/entrypoints.test.js b/src/__tests__/entrypoints.test.js new file mode 100644 index 0000000..217979c --- /dev/null +++ b/src/__tests__/entrypoints.test.js @@ -0,0 +1,19 @@ +import indexDefault, { ABsmartly, SDK } from "../index"; +import browserDefault from "../browser"; + +// Guards against the two declared package entry points (CommonJS/ES via +// `src/index.ts`, and the UMD build via `src/browser.ts`) drifting apart: +// `browser.ts` previously omitted a public API addition (the `ABsmartly` +// alias) that `index.ts` had, so `require("dist/absmartly.js").ABsmartly` +// was `undefined` in the built UMD artifact while the CJS/ES entry worked. +describe("entry point parity", () => { + it("should expose the same public API keys from index and browser default exports", () => { + expect(Object.keys(browserDefault).sort()).toEqual(Object.keys(indexDefault).sort()); + }); + + it("should expose ABsmartly as an alias for SDK from both entry points", () => { + expect(ABsmartly).toBe(SDK); + expect(indexDefault.ABsmartly).toBe(indexDefault.SDK); + expect(browserDefault.ABsmartly).toBe(browserDefault.SDK); + }); +}); diff --git a/src/__tests__/utils.test.js b/src/__tests__/utils.test.js index 87af81c..494a39d 100644 --- a/src/__tests__/utils.test.js +++ b/src/__tests__/utils.test.js @@ -314,6 +314,74 @@ describe("stringToUint8Array()", () => { } done(); }); + + describe("unmatched surrogates", () => { + // Unmatched surrogate code units are not valid UTF-8 code points; both TextEncoder + // and the manual fallback must emit U+FFFD (ef bf bd) for each one, matching the + // canonical UTF-8 replacement-character behavior. + const testCases = [ + ["lone high surrogate at end of string", "\uD800", Uint8Array.from([0xef, 0xbf, 0xbd])], + ["lone low surrogate", "\uDC00", Uint8Array.from([0xef, 0xbf, 0xbd])], + ["high surrogate followed by non-surrogate", "\uD800X", Uint8Array.from([0xef, 0xbf, 0xbd, 0x58])], + [ + "two consecutive lone high surrogates", + "\uD800\uD800", + Uint8Array.from([0xef, 0xbf, 0xbd, 0xef, 0xbf, 0xbd]), + ], + [ + "two consecutive lone low surrogates", + "\uDC00\uDC00", + Uint8Array.from([0xef, 0xbf, 0xbd, 0xef, 0xbf, 0xbd]), + ], + [ + "low surrogate followed by high surrogate (wrong order)", + "\uDC00\uD800", + Uint8Array.from([0xef, 0xbf, 0xbd, 0xef, 0xbf, 0xbd]), + ], + ["lone high surrogate after ascii", "a\uD800", Uint8Array.from([0x61, 0xef, 0xbf, 0xbd])], + ["lone low surrogate before ascii", "\uDC00b", Uint8Array.from([0xef, 0xbf, 0xbd, 0x62])], + ]; + + it("should emit U+FFFD for unmatched surrogates via the built-in TextEncoder", (done) => { + for (const [, input, expected] of testCases) { + const array = stringToUint8Array(input); + expect(Array.from(array)).toEqual(Array.from(expected)); + } + done(); + }); + + it("should emit U+FFFD for unmatched surrogates via the manual fallback", (done) => { + const OriginalTextEncoder = global.TextEncoder; + // eslint-disable-next-line no-global-assign + delete global.TextEncoder; + + try { + for (const [, input, expected] of testCases) { + const array = stringToUint8Array(input); + expect(Array.from(array)).toEqual(Array.from(expected)); + } + } finally { + global.TextEncoder = OriginalTextEncoder; + } + done(); + }); + + it("should produce identical hashUnit results for both code paths", (done) => { + const OriginalTextEncoder = global.TextEncoder; + + for (const [, input] of testCases) { + const nativeHash = hashUnit(input); + + // eslint-disable-next-line no-global-assign + delete global.TextEncoder; + const fallbackHash = hashUnit(input); + global.TextEncoder = OriginalTextEncoder; + + expect(fallbackHash).toBe(nativeHash); + } + done(); + }); + }); }); describe("base64UrlNoPadding()", () => { diff --git a/src/browser.ts b/src/browser.ts index c3399d1..450a290 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -6,4 +6,4 @@ import { ContextPublisher } from "./publisher"; // eslint-disable-next-line no-shadow import { AbortController } from "./abort"; -export default { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK }; +export default { mergeConfig, AbortController, Context, ContextDataProvider, ContextPublisher, SDK, ABsmartly: SDK }; diff --git a/src/client.ts b/src/client.ts index 95ede25..f5fd792 100644 --- a/src/client.ts +++ b/src/client.ts @@ -147,11 +147,12 @@ export default class Client { request(options: ClientRequestOptions) { let url = `${this._opts.endpoint}${options.path}`; if (options.query) { - const params = new URLSearchParams(); - for (const [key, value] of Object.entries(options.query)) { - params.append(key, String(value)); - } - const queryString = params.toString(); + // Built manually (not with URLSearchParams) because the declared IE 10 + // browser target excludes the `web.*` core-js polyfills that would + // otherwise provide it (see babel.config.js). + const queryString = Object.entries(options.query) + .map(([key, value]) => `${encodeURIComponent(key)}=${encodeURIComponent(String(value))}`) + .join("&"); if (queryString) { url = `${url}?${queryString}`; } @@ -230,11 +231,15 @@ export default class Client { return tryOnce().catch((reason: Error & { _bail?: boolean }) => { console.warn(reason); + // timeout <= 0 means "no deadline": the retry budget is governed by + // `retries` alone, so the elapsed-time check below must not apply. + const hasDeadline = timeout > 0; + if (reason._bail || retries <= 0) { throw new Error(reason.message); } else if (tries >= retries) { throw new RetryError(tries, reason, url); - } else if (waited >= timeout || reason.name === "AbortError") { + } else if ((hasDeadline && waited >= timeout) || reason.name === "AbortError") { if (tryWith.timedout) { throw new TimeoutError(timeout); } @@ -243,7 +248,7 @@ export default class Client { } let delay = (1 << tries) * this._delay + 0.5 * Math.random() * this._delay; - if (waited + delay > timeout) { + if (hasDeadline && waited + delay > timeout) { delay = timeout - waited; } diff --git a/src/context.ts b/src/context.ts index fb09ad8..abf7473 100644 --- a/src/context.ts +++ b/src/context.ts @@ -159,6 +159,7 @@ export default class Context { private _promise?: Promise; private _publishTimeout?: ReturnType; private _refreshInterval?: ReturnType; + private _flushPromise?: Promise; constructor(sdk: SDK, options: ContextOptions, params: ContextParams, promise: ContextData | Promise) { this._sdk = sdk; @@ -927,115 +928,160 @@ export default class Context { return allAttributes; } - private _flush(callback?: (error?: Error) => void, requestOptions?: ClientRequestOptions) { + private _flush( + callback?: (error?: Error) => void, + requestOptions?: ClientRequestOptions + ): Promise { if (this._publishTimeout !== undefined) { clearTimeout(this._publishTimeout); delete this._publishTimeout; } + // A flush is already in flight (it already reset `_pending` synchronously, + // so a concurrent caller can't see it via `pending()`). Wait for it to settle + // before re-evaluating, so callers like `finalize()` don't act on a queue that + // only *looks* empty because the snapshot hasn't been restored/published yet. + if (this._flushPromise) { + return this._flushPromise.then(() => this._flush(callback, requestOptions)); + } + if (this._pending === 0) { if (typeof callback === "function") { callback(); } - } else { - if (!this._failed) { - try { - const request: PublishParams = { - publishedAt: Date.now(), - units: Object.entries(this._units).map((entry) => ({ - type: entry[0], - uid: this._unitHash(entry[0]), - })), - hashed: true, - sdkVersion: SDK_VERSION, - }; - - if (this._goals.length > 0) { - request.goals = this._goals.map((x) => ({ - name: x.name, - achievedAt: x.achievedAt, - properties: x.properties, - })); - } + return Promise.resolve(undefined); + } - if (this._exposures.length > 0) { - request.exposures = this._exposures.map((x) => ({ - id: x.id, - name: x.name, - unit: x.unit, - exposedAt: x.exposedAt, - variant: x.variant, - assigned: x.assigned, - eligible: x.eligible, - overridden: x.overridden, - fullOn: x.fullOn, - custom: x.custom, - audienceMismatch: x.audienceMismatch, - ruleOverride: x.ruleOverride, - })); - } + if (this._failed) { + this._logError( + new Error( + `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` + ) + ); - const allAttributes = this._buildAttributes(); - if (allAttributes.length > 0) { - request.attributes = allAttributes; - } + this._pending = 0; + this._exposures = []; + this._goals = []; - // Snapshot and reset synchronously before the async publish. - // The data is already copied into `request` via .map(), so clearing - // immediately is safe and allows new events to accumulate during the - // in-flight publish. On failure, we restore the snapshot so the events - // are retried on the next flush cycle. - const pendingCount = this._pending; - const pendingExposures = this._exposures; - const pendingGoals = this._goals; - - this._pending = 0; - this._exposures = []; - this._goals = []; - - this._publisher - .publish(request, this._sdk, this, requestOptions) - .then(() => { - this._logEvent("publish", request); - - if (typeof callback === "function") { - callback(); - } - }) - .catch((e: Error) => { - this._pending += pendingCount; - this._exposures.push(...pendingExposures); - this._goals.push(...pendingGoals); + if (typeof callback === "function") { + callback(); + } + return Promise.resolve(undefined); + } - this._logError(e); + let request: PublishParams; + try { + request = { + publishedAt: Date.now(), + units: Object.entries(this._units).map((entry) => ({ + type: entry[0], + uid: this._unitHash(entry[0]), + })), + hashed: true, + sdkVersion: SDK_VERSION, + }; - if (typeof callback === "function") { - callback(e); - } - }); - } catch (e) { - this._logError(e as Error); + if (this._goals.length > 0) { + request.goals = this._goals.map((x) => ({ + name: x.name, + achievedAt: x.achievedAt, + properties: x.properties, + })); + } - if (typeof callback === "function") { - callback(e as Error); - } - } - } else { - this._logError( - new Error( - `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` - ) - ); + if (this._exposures.length > 0) { + request.exposures = this._exposures.map((x) => ({ + id: x.id, + name: x.name, + unit: x.unit, + exposedAt: x.exposedAt, + variant: x.variant, + assigned: x.assigned, + eligible: x.eligible, + overridden: x.overridden, + fullOn: x.fullOn, + custom: x.custom, + audienceMismatch: x.audienceMismatch, + ruleOverride: x.ruleOverride, + })); + } + + const allAttributes = this._buildAttributes(); + if (allAttributes.length > 0) { + request.attributes = allAttributes; + } + } catch (e) { + this._logError(e as Error); + + if (typeof callback === "function") { + callback(e as Error); + } + return Promise.resolve(e as Error); + } + + // Snapshot and reset synchronously before the async publish. + // The data is already copied into `request` via .map(), so clearing + // immediately is safe and allows new events to accumulate during the + // in-flight publish. On failure, we restore the snapshot so the events + // are retried on the next flush cycle. `_flushPromise` tracks this in-flight + // attempt so concurrent callers (e.g. `finalize()`) can wait on it instead of + // racing the synchronous reset above. + const pendingCount = this._pending; + const pendingExposures = this._exposures; + const pendingGoals = this._goals; + + this._pending = 0; + this._exposures = []; + this._goals = []; + + // Routing the publisher call through a shared handler normalizes a + // synchronously throwing custom publisher into the same restore/reject path + // as an async rejection, so the snapshot is restored either way. The call + // itself stays synchronous (no extra microtask hop) so timer-driven callers + // observe the publish attempt within the same tick, as before. + const onFailure = (e: Error): Error => { + this._pending += pendingCount; + this._exposures.push(...pendingExposures); + this._goals.push(...pendingGoals); - this._pending = 0; - this._exposures = []; - this._goals = []; + this._logError(e); + + // Reschedule automatic delivery for the restored batch; this is a no-op + // unless publishDelay >= 0 and no timer is already pending. + this._setTimeout(); + + if (typeof callback === "function") { + callback(e); + } + + return e; + }; + + let publishResult: Promise; + try { + publishResult = this._publisher.publish(request, this._sdk, this, requestOptions); + } catch (e) { + const result = onFailure(e as Error); + this._flushPromise = undefined; + return Promise.resolve(result); + } + + this._flushPromise = publishResult + .then(() => { + this._logEvent("publish", request); if (typeof callback === "function") { callback(); } - } - } + + return undefined; + }) + .catch((e: Error) => onFailure(e)) + .finally(() => { + this._flushPromise = undefined; + }); + + return this._flushPromise; } private _refresh(callback?: (error?: Error) => void, requestOptions?: ClientRequestOptions) { @@ -1153,41 +1199,46 @@ export default class Context { } private _finalize(requestOptions?: ClientRequestOptions) { - if (!this._finalized) { - if (!this._finalizing) { - if (this._refreshInterval !== undefined) { - clearInterval(this._refreshInterval); - delete this._refreshInterval; - } + if (this._finalized) { + return Promise.resolve(); + } - if (this.pending() > 0) { - this._finalizing = new Promise((resolve, reject) => { - this._flush((error) => { - this._finalizing = null; + if (this._finalizing) { + return this._finalizing; + } - if (error) { - reject(error); - } else { - this._finalized = true; - this._logEvent("finalize"); + if (this._refreshInterval !== undefined) { + clearInterval(this._refreshInterval); + delete this._refreshInterval; + } - resolve(); - } - }, requestOptions); - }); + // `pending() === 0` alone is not sufficient: `_flush` resets `_pending` + // synchronously before its publish settles, so a flush can be in flight + // while the queue already looks empty. Only take the synchronous fast + // path when nothing is pending AND no flush is in progress; otherwise + // fall through to `_flush`, which itself waits for any in-flight attempt. + if (this._pending === 0 && !this._flushPromise) { + this._finalized = true; + this._logEvent("finalize"); - return this._finalizing; - } + return Promise.resolve(); + } - this._finalized = true; - this._logEvent("finalize"); + this._finalizing = new Promise((resolve, reject) => { + this._flush((error) => { + this._finalizing = null; - return Promise.resolve(); - } + if (error) { + reject(error); + } else { + this._finalized = true; + this._logEvent("finalize"); - return this._finalizing; - } + resolve(); + } + }, requestOptions); + }); - return Promise.resolve(); + return this._finalizing; } } diff --git a/src/utils.ts b/src/utils.ts index 512089f..b50aa6b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -156,12 +156,18 @@ export function stringToUint8Array(value: string) { const utf8: number[] = []; for (let i = 0; i < value.length; i++) { let c = value.charCodeAt(i); - if (c >= 0xd800 && c <= 0xdbff && i + 1 < value.length) { - const next = value.charCodeAt(i + 1); + if (c >= 0xd800 && c <= 0xdbff) { + const next = i + 1 < value.length ? value.charCodeAt(i + 1) : 0; if (next >= 0xdc00 && next <= 0xdfff) { c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000; i++; + } else { + // Unmatched high surrogate: not a valid UTF-8 code point, encode as U+FFFD. + c = 0xfffd; } + } else if (c >= 0xdc00 && c <= 0xdfff) { + // Unmatched low surrogate: not a valid UTF-8 code point, encode as U+FFFD. + c = 0xfffd; } if (c < 0x80) { utf8.push(c); From 576dd3b2de495d72ce82eb6a622e49a44b6f4917 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 12:41:29 +0100 Subject: [PATCH 15/25] fix: address third-round P2 review feedback (IE10-safe promise chain, sync-failure recovery, surrogate-safe query encoding) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - context.ts: avoid Promise.prototype.finally() in _flush() (not part of the ES6 Promise contract the documented IE 10 polyfill guidance relies on); use the two-argument .then(onSuccess, onFailure) form and clear _flushPromise in both branches instead. - context.ts: fix _finalize() leaving isFinalizing() stuck true after a synchronously throwing custom publisher — the flush callback could previously run (and clear _finalizing) before the `new Promise(...)` expression assigning to it had finished evaluating, so the assignment clobbered the clear. Now the deferred's resolve/reject are captured and this._finalizing is assigned before _flush is called. - context.ts: isolate a throwing custom eventLogger on the publish success path so it can't be misrouted into the failure handler, which would incorrectly restore and resend an already-delivered batch. - utils.ts: add toWellFormedString(), replacing unmatched UTF-16 surrogates with U+FFFD (matching String.prototype.toWellFormed(), not assumed available under the declared targets). - client.ts: normalize query keys/values through toWellFormedString() before encodeURIComponent(), which throws URIError on an unpaired surrogate where URLSearchParams previously degraded gracefully. - local-server-integration.test.js: release idle keep-alive sockets during teardown for determinism on Node < 19. Co-authored-by: Pedro-Revez-Silva Co-authored-by: jervasion-absmartly --- src/__tests__/client.test.js | 25 ++++++ src/__tests__/context.test.js | 79 +++++++++++++++++ .../local-server-integration.test.js | 1 + src/__tests__/utils.test.js | 34 ++++++++ src/client.ts | 10 ++- src/context.ts | 87 +++++++++++++------ src/utils.ts | 27 ++++++ 7 files changed, 235 insertions(+), 28 deletions(-) diff --git a/src/__tests__/client.test.js b/src/__tests__/client.test.js index d029986..6fb3ae3 100644 --- a/src/__tests__/client.test.js +++ b/src/__tests__/client.test.js @@ -687,6 +687,31 @@ describe("Client", () => { }); }); + it("request() should not throw on unmatched surrogates in query parameters", (done) => { + fetch.mockResolvedValueOnce(responseMock(200, "OK", defaultMockResponse)); + + const client = new Client(clientOptions); + + // A lone UTF-16 surrogate is accepted JavaScript string content but is not + // valid UTF-8; unlike URLSearchParams (which substitutes U+FFFD), + // encodeURIComponent() throws URIError on it directly, so it must be + // normalized to well-formed UTF-16 first. + client + .request({ + method: "GET", + path: "/context", + query: { application: "\uD800" }, + }) + .then((response) => { + expect(fetch).toHaveBeenCalledTimes(1); + expect(fetch).toHaveBeenLastCalledWith(`${endpoint}/context?application=%EF%BF%BD`, expect.any(Object)); + + expect(response).toEqual(defaultMockResponse); + + done(); + }); + }); + it("request() should omit query parameters if dict empty", (done) => { fetch.mockResolvedValueOnce(responseMock(200, "OK", defaultMockResponse)); diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 2b3867b..86071b8 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4159,6 +4159,46 @@ describe("Context", () => { expect(context.isFinalizing()).toEqual(true); expect(() => context.publish()).toThrow(); }); + + it("should not restore or resend an already-delivered batch when a custom eventLogger throws on the publish success event", (done) => { + const observerError = new Error("eventLogger failure"); + const throwingEventLogger = jest.fn((_, eventName) => { + if (eventName === "publish") { + throw observerError; + } + }); + + const context = new Context( + sdk, + { ...contextOptions, eventLogger: throwingEventLogger }, + contextParams, + getContextResponse + ); + + context.track("goal1", { amount: 125 }); + expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + context.publish().then(() => { + // The batch was already delivered successfully; the observer's throw + // must not be treated as a publish failure that restores the queue + // and resends the batch on the next flush. + expect(context.pending()).toEqual(0); + expect(consoleErrorSpy).toHaveBeenCalledWith(observerError); + + publisher.publish.mockClear(); + + context.publish().then(() => { + expect(publisher.publish).not.toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + done(); + }); + }); + }); }); describe("finalize()", () => { @@ -4522,6 +4562,45 @@ describe("Context", () => { }); }); + it("should clear isFinalizing() and allow a retry after a synchronously throwing publisher", (done) => { + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0 }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + const syncError = new Error("synchronous publisher failure"); + publisher.publish.mockImplementationOnce(() => { + throw syncError; + }); + + context.finalize().catch((e) => { + expect(e).toBe(syncError); + // A `finalize()` callback that fires synchronously (as it does here, + // since the publisher throws before any microtask boundary) must not + // leave isFinalizing() stuck true — otherwise the context can never + // finalize or retry. + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(false); + expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.finalize().then(() => { + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(true); + expect(context.pending()).toEqual(0); + expect(publisher.publish).toHaveBeenCalledTimes(2); + + done(); + }); + }); + }); + it("should reschedule the automatic publish timer after a scheduled flush fails", (done) => { jest.useFakeTimers("legacy"); jest.spyOn(global, "setTimeout"); diff --git a/src/__tests__/local-server-integration.test.js b/src/__tests__/local-server-integration.test.js index 34f8fc8..83d7387 100644 --- a/src/__tests__/local-server-integration.test.js +++ b/src/__tests__/local-server-integration.test.js @@ -47,6 +47,7 @@ describe("Local server integration (real HTTP)", () => { afterAll((done) => { server.close(done); + server.closeIdleConnections?.(); }); it("performs a real GET /context and PUT /context against a local server", async () => { diff --git a/src/__tests__/utils.test.js b/src/__tests__/utils.test.js index 494a39d..62aec2b 100644 --- a/src/__tests__/utils.test.js +++ b/src/__tests__/utils.test.js @@ -8,6 +8,7 @@ import { isObject, isPromise, stringToUint8Array, + toWellFormedString, } from "../utils"; class SomeClass {} @@ -420,3 +421,36 @@ describe("base64UrlNoPadding()", () => { done(); }); }); + +describe("toWellFormedString()", () => { + it("should leave well-formed strings unchanged", (done) => { + expect(toWellFormedString("")).toBe(""); + expect(toWellFormedString("normal string")).toBe("normal string"); + expect(toWellFormedString("açb↓c")).toBe("açb↓c"); + expect(toWellFormedString("😀")).toBe("😀"); + expect(toWellFormedString("a😀b")).toBe("a😀b"); + + done(); + }); + + it("should replace unmatched surrogates with U+FFFD", (done) => { + expect(toWellFormedString("\uD800")).toBe("�"); + expect(toWellFormedString("\uDC00")).toBe("�"); + expect(toWellFormedString("\uD800X")).toBe("�X"); + expect(toWellFormedString("a\uD800b")).toBe("a�b"); + expect(toWellFormedString("\uD800\uD800")).toBe("��"); + expect(toWellFormedString("\uDC00\uD800")).toBe("��"); + + done(); + }); + + it("should always be safe to pass to encodeURIComponent()", (done) => { + const inputs = ["\uD800", "\uDC00", "\uD800X", "a\uD800b", "\uD800\uD800", "\uDC00\uD800", "normal", "😀"]; + + for (const input of inputs) { + expect(() => encodeURIComponent(toWellFormedString(input))).not.toThrow(); + } + + done(); + }); +}); diff --git a/src/client.ts b/src/client.ts index f5fd792..c0f5fe1 100644 --- a/src/client.ts +++ b/src/client.ts @@ -7,6 +7,7 @@ import { AbortError, RetryError, TimeoutError } from "./errors"; import { type AbortSignal as ABsmartlyAbortSignal } from "./abort-controller-shim"; import { type ContextOptions, type ContextParams } from "./context"; import { type PublishParams } from "./publisher"; +import { toWellFormedString } from "./utils"; export type FetchResponse = { status: number; @@ -149,9 +150,14 @@ export default class Client { if (options.query) { // Built manually (not with URLSearchParams) because the declared IE 10 // browser target excludes the `web.*` core-js polyfills that would - // otherwise provide it (see babel.config.js). + // otherwise provide it (see babel.config.js). Values are normalized to + // well-formed UTF-16 first: unlike `URLSearchParams`, `encodeURIComponent` + // throws `URIError: URI malformed` on an unpaired surrogate. const queryString = Object.entries(options.query) - .map(([key, value]) => `${encodeURIComponent(key)}=${encodeURIComponent(String(value))}`) + .map( + ([key, value]) => + `${encodeURIComponent(toWellFormedString(key))}=${encodeURIComponent(toWellFormedString(String(value)))}` + ) .join("&"); if (queryString) { url = `${url}?${queryString}`; diff --git a/src/context.ts b/src/context.ts index abf7473..9396875 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1057,6 +1057,25 @@ export default class Context { return e; }; + // The batch has already been delivered by this point, so an exception from + // observing that success (e.g. a throwing custom eventLogger) must not be + // treated as a publish failure — that would incorrectly restore and resend + // an already-delivered batch. `callback()` still runs unconditionally + // afterward so `publish()`/`finalize()`'s own promise settles either way. + const onSuccess = (): undefined => { + try { + this._logEvent("publish", request); + } catch (observerError) { + console.error(observerError); + } + + if (typeof callback === "function") { + callback(); + } + + return undefined; + }; + let publishResult: Promise; try { publishResult = this._publisher.publish(request, this._sdk, this, requestOptions); @@ -1066,20 +1085,14 @@ export default class Context { return Promise.resolve(result); } - this._flushPromise = publishResult - .then(() => { - this._logEvent("publish", request); - - if (typeof callback === "function") { - callback(); - } - - return undefined; - }) - .catch((e: Error) => onFailure(e)) - .finally(() => { - this._flushPromise = undefined; - }); + // Neither `onSuccess` nor `onFailure` throws, so `.then(onSuccess, + // onFailure)` never rejects and clearing `_flushPromise` only needs a + // fulfillment handler. `.finally()` is avoided (not part of the ES6 + // Promise contract the documented IE 10 target relies on a polyfill for). + this._flushPromise = publishResult.then(onSuccess, onFailure).then((result) => { + this._flushPromise = undefined; + return result; + }); return this._flushPromise; } @@ -1224,21 +1237,43 @@ export default class Context { return Promise.resolve(); } - this._finalizing = new Promise((resolve, reject) => { - this._flush((error) => { + // Assign `this._finalizing` BEFORE calling `_flush`, using a manually + // created deferred rather than passing a callback into a `new + // Promise(executor)`. `_flush`'s callback can fire synchronously (e.g. + // from a synchronously throwing custom publisher, or the already-failed + // fast path), and if it ran inside a `new Promise((resolve, reject) => { + // this._flush(callback...) })` executor, it would run — and clear + // `this._finalizing` — before that `new Promise(...)` expression itself + // finished evaluating; the outer `this._finalizing = ...` assignment + // would then immediately clobber the clear back to a non-null value, + // leaving `isFinalizing()` stuck `true` forever. Setting `_finalizing` + // up front, before `_flush` is even called, avoids that ordering + // entirely: whether the callback fires synchronously or asynchronously, + // `this._finalizing` is already the promise being resolved below. + let resolveFinalizing!: () => void; + let rejectFinalizing!: (error: Error) => void; + const finalizing = new Promise((resolve, reject) => { + resolveFinalizing = resolve; + rejectFinalizing = reject; + }); + + this._finalizing = finalizing; + + this._flush((error) => { + if (this._finalizing === finalizing) { this._finalizing = null; + } - if (error) { - reject(error); - } else { - this._finalized = true; - this._logEvent("finalize"); + if (error) { + rejectFinalizing(error); + } else { + this._finalized = true; + this._logEvent("finalize"); - resolve(); - } - }, requestOptions); - }); + resolveFinalizing(); + } + }, requestOptions); - return this._finalizing; + return finalizing; } } diff --git a/src/utils.ts b/src/utils.ts index b50aa6b..a1671bb 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -148,6 +148,33 @@ export function arrayEqualsShallow(a?: unknown[], b?: unknown[]) { return a === b || (a?.length === b?.length && !a?.some((va, vi) => b && va !== b[vi])); } +// Replaces unmatched UTF-16 surrogates with U+FFFD, matching +// `String.prototype.toWellFormed()` (ES2024, not assumed available under the +// declared Node 6 / IE 10 targets) so `encodeURIComponent` — which throws +// `URIError: URI malformed` on a lone surrogate — never sees one. +export function toWellFormedString(value: string): string { + let result = ""; + + for (let i = 0; i < value.length; i++) { + const c = value.charCodeAt(i); + if (c >= 0xd800 && c <= 0xdbff) { + const next = i + 1 < value.length ? value.charCodeAt(i + 1) : 0; + if (next >= 0xdc00 && next <= 0xdfff) { + result += value[i] + value[i + 1]; + i++; + } else { + result += "�"; + } + } else if (c >= 0xdc00 && c <= 0xdfff) { + result += "�"; + } else { + result += value[i]; + } + } + + return result; +} + export function stringToUint8Array(value: string) { if (typeof TextEncoder !== "undefined") { return new TextEncoder().encode(value); From 2777c8b41dafb88cce70dfd656a3c4f90618a3e6 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 12:56:27 +0100 Subject: [PATCH 16/25] docs, test: address remaining review feedback from calthejuggler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README.md: drop Android and Rust SDK links (no reference in the cross-sdk-tests wrapper set actually shipped) and the Dart SDK link (dart-sdk repo returns 404; only a dart-wrapper exists in cross-sdk-tests, no standalone published SDK yet). - src/__tests__/constructor-options.test.js: drop the redundant file header comment (the describe block name already says the same thing). - src/context.ts: reword the override()-after-finalize() comment — "parity with the production SDK" was ambiguous (JS is itself a production SDK). Point at the actual source of truth instead: cross-sdk-tests scenario "190 - Post-Finalize - override() Allowed (Verified Finalized)", which is explicitly JS-specific ("JS parity" in its own description). - src/context.ts: fix an inaccurate comment on the indeterminate- audience cache check — it claimed the mismatch flag is left at "false", but it's actually left at whatever it was previously cached as. - src/__tests__/context.test.js: give the "should clear assignment cache when experiment ID changes" test a seed (seedHi=1, seedLo=3) that resolves to a different variant (1) than expectedVariants["exp_test_abc"] (2), so the assertion actually proves the cache was recomputed rather than incidentally re-serving a still-valid cached variant. --- README.md | 3 --- src/__tests__/constructor-options.test.js | 6 ------ src/__tests__/context.test.js | 10 +++++++--- src/context.ts | 14 +++++++++----- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 23133ca..76506fc 100644 --- a/README.md +++ b/README.md @@ -549,13 +549,10 @@ When an audience cannot be evaluated to a boolean (a malformed or non-boolean fi - [Vue2 SDK](https://www.github.com/absmartly/vue2-sdk) - [Vue3 SDK](https://www.github.com/absmartly/vue3-sdk) - [Java SDK](https://www.github.com/absmartly/java-sdk) -- [Android SDK](https://www.github.com/absmartly/android-sdk) - [Swift SDK](https://www.github.com/absmartly/swift-sdk) -- [Dart SDK](https://www.github.com/absmartly/dart-sdk) - [Flutter SDK](https://www.github.com/absmartly/flutter-sdk) - [PHP SDK](https://www.github.com/absmartly/php-sdk) - [Python3 SDK](https://www.github.com/absmartly/python3-sdk) - [Go SDK](https://www.github.com/absmartly/go-sdk) - [Ruby SDK](https://www.github.com/absmartly/ruby-sdk) - [.NET SDK](https://www.github.com/absmartly/dotnet-sdk) -- [Rust SDK](https://www.github.com/absmartly/rust-sdk) diff --git a/src/__tests__/constructor-options.test.js b/src/__tests__/constructor-options.test.js index 4e0b437..d857835 100644 --- a/src/__tests__/constructor-options.test.js +++ b/src/__tests__/constructor-options.test.js @@ -1,9 +1,3 @@ -// Constructor option-merging tests for SDK and Client. -// -// These deliberately use the real SDK and Client constructors (no jest.mock), -// so they exercise the actual _extractClientOptions / option-merge logic -// rather than Jest doubles, which would make the assertions vacuous. - import SDK from "../sdk"; import Client from "../client"; diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 86071b8..e983010 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -1136,8 +1136,12 @@ describe("Context", () => { id: 11, trafficSeedHi: 54870830, trafficSeedLo: 398724581, - seedHi: 77498863, - seedLo: 34737352, + // Chosen so the resulting variant (1) differs from + // expectedVariants["exp_test_abc"] (2): proves the cache was + // actually recomputed with the new seed, not just re-serving a + // stale cached assignment that happens to still be valid. + seedHi: 1, + seedLo: 3, }; } return x; @@ -1147,7 +1151,7 @@ describe("Context", () => { provider.getContextData.mockReturnValue(Promise.resolve(refreshWithChangedId)); context.refresh().then(() => { - expect(context.treatment("exp_test_abc")).toEqual(2); + expect(context.treatment("exp_test_abc")).toEqual(1); expect(context.treatment("not_found")).toEqual(0); expect(context.pending()).toEqual(3); diff --git a/src/context.ts b/src/context.ts index 9396875..ed86811 100644 --- a/src/context.ts +++ b/src/context.ts @@ -415,8 +415,10 @@ export default class Context { } override(experimentName: string, variant: number) { - // override() is allowed after finalize() (parity with the production SDK, - // which sets overrides unconditionally). + // Deliberately allowed after finalize() — not guarded by + // _checkNotFinalized(), unlike track()/treatment()/etc. This is the + // canonical cross-SDK behavior for this SDK: see cross-sdk-tests + // scenario "190 - Post-Finalize - override() Allowed (Verified Finalized)". this._overrides = Object.assign(this._overrides, { [experimentName]: variant }); } @@ -545,9 +547,11 @@ export default class Context { if (!assignment.ruleOverride && experiment.audience && experiment.audience.length > 0) { const result = this._evaluateAudience(experiment.audience); - // Mirror the assignment-time logic: a null result leaves the - // mismatch flag unchanged (false), so the cached assignment - // stays valid rather than being needlessly invalidated. + // An indeterminate (null) audience result leaves the cached + // `audienceMismatch` flag as-is rather than forcing it to `false`, + // so a cached mismatch=true assignment isn't wrongly treated as + // stale — this only affects the cache-validity check below, not + // the flag's value. const newAudienceMismatch = result !== null ? !result : assignment.audienceMismatch; if (newAudienceMismatch !== assignment.audienceMismatch) { From 77b582199ac137c2f41ef2568bade813ee54a7a1 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 13:05:33 +0100 Subject: [PATCH 17/25] fix: protect finalize()/_flush() from throwing custom eventLogger; fix client.test.js promise propagation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - context.ts: a throwing custom eventLogger on the "finalize" event could leave finalize()'s promise permanently unsettled — the throw from _logEvent("finalize") happened before resolveFinalizing(), and propagated up through _flush's unguarded callback(), rejecting _flush's internal promise chain and leaving _flushPromise stuck. Reordered _finalize's success branch to resolve before logging, and wrapped _logError/callback invocations in _flush's onFailure/onSuccess in try/catch so an observer exception can never prevent the flush promise from settling or strand _flushPromise. - src/__tests__/context.test.js: added regression coverage for a throwing eventLogger on both the finalize-success and publish- failure paths during finalize(); confirmed both hang/timeout against the pre-fix code and pass after. - src/__tests__/client.test.js: the unmatched-surrogates query test didn't return or handle its request() promise chain, so a rejection would time out instead of failing with a clear error. Return the chain instead of using an unhandled done() callback. --- src/__tests__/client.test.js | 6 +-- src/__tests__/context.test.js | 73 +++++++++++++++++++++++++++++++++++ src/context.ts | 67 +++++++++++++++++++++++++------- 3 files changed, 129 insertions(+), 17 deletions(-) diff --git a/src/__tests__/client.test.js b/src/__tests__/client.test.js index 6fb3ae3..ca943c4 100644 --- a/src/__tests__/client.test.js +++ b/src/__tests__/client.test.js @@ -687,7 +687,7 @@ describe("Client", () => { }); }); - it("request() should not throw on unmatched surrogates in query parameters", (done) => { + it("request() should not throw on unmatched surrogates in query parameters", () => { fetch.mockResolvedValueOnce(responseMock(200, "OK", defaultMockResponse)); const client = new Client(clientOptions); @@ -696,7 +696,7 @@ describe("Client", () => { // valid UTF-8; unlike URLSearchParams (which substitutes U+FFFD), // encodeURIComponent() throws URIError on it directly, so it must be // normalized to well-formed UTF-16 first. - client + return client .request({ method: "GET", path: "/context", @@ -707,8 +707,6 @@ describe("Client", () => { expect(fetch).toHaveBeenLastCalledWith(`${endpoint}/context?application=%EF%BF%BD`, expect.any(Object)); expect(response).toEqual(defaultMockResponse); - - done(); }); }); diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index e983010..0308bca 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4605,6 +4605,79 @@ describe("Context", () => { }); }); + it("should settle the finalize() promise even when a custom eventLogger throws on the finalize event", (done) => { + const throwingEventLogger = jest.fn((_, eventName) => { + if (eventName === "finalize") { + throw new Error("eventLogger boom on finalize"); + } + }); + + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0, eventLogger: throwingEventLogger }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + // A throwing eventLogger on the "finalize" event must not prevent + // finalize() from resolving, or leave isFinalizing()/isFinalized() stuck. + context.finalize().then(() => { + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(true); + expect(consoleErrorSpy).toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + done(); + }); + }); + + it("should not get stuck when a custom eventLogger throws on the error event during finalize()", (done) => { + const throwingEventLogger = jest.fn((_, eventName) => { + if (eventName === "error") { + throw new Error("eventLogger boom on error"); + } + }); + + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0, eventLogger: throwingEventLogger }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.reject(new Error("transport failed"))); + + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + context.finalize().catch((e) => { + expect(e.message).toEqual("transport failed"); + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(false); + expect(context.pending()).toEqual(1); + expect(consoleErrorSpy).toHaveBeenCalled(); + + // Retry must still work — the flush must not be left permanently stuck. + publisher.publish.mockReturnValue(Promise.resolve()); + + context.finalize().then(() => { + expect(context.isFinalized()).toEqual(true); + + consoleErrorSpy.mockRestore(); + done(); + }); + }); + }); + it("should reschedule the automatic publish timer after a scheduled flush fails", (done) => { jest.useFakeTimers("legacy"); jest.spyOn(global, "setTimeout"); diff --git a/src/context.ts b/src/context.ts index ed86811..a62c795 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1048,14 +1048,29 @@ export default class Context { this._exposures.push(...pendingExposures); this._goals.push(...pendingGoals); - this._logError(e); + try { + this._logError(e); + } catch (observerError) { + console.error(observerError); + } // Reschedule automatic delivery for the restored batch; this is a no-op // unless publishDelay >= 0 and no timer is already pending. this._setTimeout(); + // `callback` is internal glue (from `publish()`/`finalize()`), but it can + // itself invoke a user-supplied eventLogger (see `_finalize`'s callback, + // which calls `_logEvent("finalize")`). A throw there must not propagate + // into this promise chain: `_flushPromise` is cleared unconditionally + // below regardless of whether `onFailure`/`onSuccess` throw, but an + // unguarded throw here would still skip the `callback(e)` call's own + // completion and any code after it in the caller. if (typeof callback === "function") { - callback(e); + try { + callback(e); + } catch (observerError) { + console.error(observerError); + } } return e; @@ -1074,7 +1089,11 @@ export default class Context { } if (typeof callback === "function") { - callback(); + try { + callback(); + } catch (observerError) { + console.error(observerError); + } } return undefined; @@ -1089,14 +1108,22 @@ export default class Context { return Promise.resolve(result); } - // Neither `onSuccess` nor `onFailure` throws, so `.then(onSuccess, - // onFailure)` never rejects and clearing `_flushPromise` only needs a - // fulfillment handler. `.finally()` is avoided (not part of the ES6 - // Promise contract the documented IE 10 target relies on a polyfill for). - this._flushPromise = publishResult.then(onSuccess, onFailure).then((result) => { - this._flushPromise = undefined; - return result; - }); + // Neither `onSuccess` nor `onFailure` throws (both isolate observer + // exceptions internally), so `.then(onSuccess, onFailure)` never rejects. + // The `_flushPromise` cleanup below still uses the two-argument `.then()` + // form defensively, so a flush can never get stuck referencing a settled + // promise. `.finally()` is avoided (not part of the ES6 Promise contract + // the documented IE 10 target relies on a polyfill for). + this._flushPromise = publishResult.then(onSuccess, onFailure).then( + (result) => { + this._flushPromise = undefined; + return result; + }, + (e) => { + this._flushPromise = undefined; + throw e; + } + ); return this._flushPromise; } @@ -1236,7 +1263,12 @@ export default class Context { // fall through to `_flush`, which itself waits for any in-flight attempt. if (this._pending === 0 && !this._flushPromise) { this._finalized = true; - this._logEvent("finalize"); + + try { + this._logEvent("finalize"); + } catch (observerError) { + console.error(observerError); + } return Promise.resolve(); } @@ -1272,9 +1304,18 @@ export default class Context { rejectFinalizing(error); } else { this._finalized = true; - this._logEvent("finalize"); + // Settle the finalize promise before logging the event: a throwing + // custom eventLogger must not prevent `finalizing` from resolving + // (which would strand `isFinalizing()`/`isFinalized()` and any + // awaiters forever). resolveFinalizing(); + + try { + this._logEvent("finalize"); + } catch (observerError) { + console.error(observerError); + } } }, requestOptions); From e6db82583aabf200b475f8ffe248d86006043d2d Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 15:26:41 +0100 Subject: [PATCH 18/25] fix: isolate observer failures on the failed-init discard path in _flush() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The _failed branch of _flush() (discarding queued events after a failed context initialization) called _logError() unguarded, unlike the transport-failure path below it. A throwing custom eventLogger there threw synchronously out of _flush() itself, before it could clear _pending/_exposures/_goals or invoke the callback — which meant _finalize()'s callback never ran, permanently stranding `_finalizing` with no settlement (worse than a rejection: finalize() would hang forever, and no code path could recover). Wrapped both the _logError() call and the callback invocation in try/catch, matching the pattern already used for the transport-failure and success paths. Added a regression test that reproduces a throwing eventLogger during the discard-after-failed-init flow through finalize() — confirmed it hangs against the pre-fix code and settles correctly (isFinalized: true, pending: 0) after the fix. --- src/__tests__/context.test.js | 40 +++++++++++++++++++++++++++++++++++ src/context.ts | 25 ++++++++++++++++------ 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 0308bca..29566f7 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4410,6 +4410,46 @@ describe("Context", () => { }); }); + it("should still settle finalize() when a custom eventLogger throws while discarding events after failed initialization", (done) => { + // The constructor's own ready-rejection handler also calls the + // eventLogger with "error" — only start throwing after that call, so + // this isolates the discard path inside _flush()/_finalize(). + const throwingEventLogger = jest.fn((_, eventName) => { + if (eventName === "error" && throwingEventLogger.mock.calls.length > 1) { + throw new Error("eventLogger boom on error"); + } + }); + + const context = new Context( + sdk, + { ...contextOptions, eventLogger: throwingEventLogger }, + contextParams, + Promise.reject("bad request error text") + ); + + context.ready().then(() => { + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + // Discarding queued events after a failed init must still settle + // finalize() (and clear the queue) even when the eventLogger throws — + // otherwise `_finalizing` is left referencing a promise that never + // resolves or rejects. + context.finalize().then(() => { + expect(publisher.publish).not.toHaveBeenCalled(); + expect(context.pending()).toEqual(0); + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(true); + expect(consoleErrorSpy).toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + done(); + }); + }); + }); + it("should return current promise when called twice", (done) => { const context = new Context(sdk, contextOptions, contextParams, getContextResponse); diff --git a/src/context.ts b/src/context.ts index a62c795..0761700 100644 --- a/src/context.ts +++ b/src/context.ts @@ -957,18 +957,31 @@ export default class Context { } if (this._failed) { - this._logError( - new Error( - `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` - ) - ); + // Guarded the same way as the transport-failure path below: a throwing + // custom eventLogger here must not propagate synchronously out of + // `_flush` — that would skip clearing `_pending`/`_exposures`/`_goals` + // and (via `_finalize`'s callback never running) permanently strand + // `_finalizing` with no settlement. + try { + this._logError( + new Error( + `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` + ) + ); + } catch (observerError) { + console.error(observerError); + } this._pending = 0; this._exposures = []; this._goals = []; if (typeof callback === "function") { - callback(); + try { + callback(); + } catch (observerError) { + console.error(observerError); + } } return Promise.resolve(undefined); } From 1bdcf9ef43a0b4b8a6ab1425f08c7e81a7441633 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 10 Sep 2026 10:57:55 +0100 Subject: [PATCH 19/25] fix: keep ready() always resolving true; restore failed batches in chronological order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - context.ts: centralized observer-exception isolation into _logEvent()/_logError() themselves (catch + console.error), instead of scattering try/catch at each call site. This closes two related bugs the previous per-call-site guards missed: * a throwing custom eventLogger on the init-rejection path caused the internal ready promise to reject, so `await context.ready()` rejected instead of always resolving `true` as documented (the v2 migration guide's contract) — isFailed()/readyError() were still set correctly, but callers following the documented "ready() never rejects" contract would break. * a throwing eventLogger on the *successful* ready path was caught by the adjacent .catch() on the same promise chain, incorrectly marking a successfully-initialized context as failed. Simplified the now-redundant local try/catch wrappers in _flush()/ _finalize() added in the previous fix, since _logEvent()/_logError() can no longer throw. - context.ts: _flush()'s onFailure restored a failed batch by push()ing the pre-publish snapshot onto the current queues. Because the snapshot is taken and the queues are cleared synchronously before the async publish resolves, any event recorded while that publish was in flight ends up in the queue before the restore runs — push() then puts the older, already-recorded events after the newer ones, delivering them out of chronological order to the collector on retry. Changed to prepend (concat older ahead of newer) for both exposures and goals. - src/__tests__/context.test.js: added regression coverage for a throwing eventLogger on both ready() failure and success paths, and for chronological-order preservation when a failed batch is restored alongside newer events recorded during the in-flight publish. All reproduced the reported bugs against the pre-fix code. --- src/__tests__/context.test.js | 105 +++++++++++++++++++++++++++++ src/context.ts | 121 +++++++++++++++------------------- 2 files changed, 158 insertions(+), 68 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 29566f7..3644e33 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4606,6 +4606,50 @@ describe("Context", () => { }); }); + it("should restore a failed batch ahead of events recorded during the in-flight publish, preserving chronological order", (done) => { + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0 }, + contextParams, + getContextResponse + ); + + context.track("old_goal"); + expect(context.pending()).toEqual(1); + + let rejectFirst; + publisher.publish.mockReturnValueOnce( + new Promise((resolve, reject) => { + rejectFirst = reject; + }) + ); + + const firstPublish = context.publish(); + + // Record a newer event while the first publish is still in flight (its + // snapshot was already taken and _goals/_exposures were reset). + context.track("new_goal"); + + rejectFirst(new Error("transport failed")); + + firstPublish.catch((e) => { + expect(e.message).toEqual("transport failed"); + expect(context.pending()).toEqual(2); + + publisher.publish.mockReturnValueOnce(Promise.resolve()); + + context.publish().then(() => { + const retryRequest = publisher.publish.mock.calls[1][0]; + // The restored (older) batch must come before the newer event, not + // after it — otherwise the collector sees goals out of chronological + // order. + expect(retryRequest.goals.map((g) => g.name)).toEqual(["old_goal", "new_goal"]); + + done(); + }); + }); + }); + it("should clear isFinalizing() and allow a retry after a synchronously throwing publisher", (done) => { const context = new Context( sdk, @@ -5518,6 +5562,67 @@ describe("Context input handling and lifecycle regressions", () => { expect(() => context.track("goal_name")).not.toThrow(); expect(() => context.attribute("attr", "value")).not.toThrow(); }); + + it("should still resolve true and record the error when a custom eventLogger throws on init failure", async () => { + const initError = new Error("fetch failed"); + const throwingEventLogger = jest.fn((_, eventName) => { + if (eventName === "error") { + throw new Error("eventLogger boom on error"); + } + }); + + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + const context = new Context( + newMockSDK(), + { ...contextOptions, eventLogger: throwingEventLogger }, + contextParams, + Promise.reject(initError) + ); + + // The v2 migration guide promises ready() always resolves true; a + // throwing observer on the "error" event must not turn that into a + // rejection. + const result = await context.ready(); + + expect(result).toBe(true); + expect(context.isFailed()).toBe(true); + expect(context.readyError()).toBe(initError); + expect(consoleErrorSpy).toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + }); + + it("should not mark a successful init as failed when a custom eventLogger throws on the ready event", async () => { + const throwingEventLogger = jest.fn((_, eventName) => { + if (eventName === "ready") { + throw new Error("eventLogger boom on ready"); + } + }); + + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + + const context = new Context( + newMockSDK(), + { ...contextOptions, eventLogger: throwingEventLogger }, + contextParams, + Promise.resolve({ experiments: [] }) + ); + + // The success handler runs `this._logEvent("ready", data)`; an observer + // exception there sits between a `.then()` and the constructor's own + // `.catch()` on the same promise chain, so an unguarded throw would + // incorrectly route through the failure branch and mark this a failed + // init even though the fetch itself succeeded. + const result = await context.ready(); + + expect(result).toBe(true); + expect(context.isFailed()).toBe(false); + expect(context.readyError()).toBe(null); + expect(consoleErrorSpy).toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + }); }); describe("variable resolution over experiment arrays", () => { diff --git a/src/context.ts b/src/context.ts index 0761700..a22e043 100644 --- a/src/context.ts +++ b/src/context.ts @@ -957,31 +957,24 @@ export default class Context { } if (this._failed) { - // Guarded the same way as the transport-failure path below: a throwing - // custom eventLogger here must not propagate synchronously out of - // `_flush` — that would skip clearing `_pending`/`_exposures`/`_goals` - // and (via `_finalize`'s callback never running) permanently strand - // `_finalizing` with no settlement. - try { - this._logError( - new Error( - `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` - ) - ); - } catch (observerError) { - console.error(observerError); - } + // _logError() isolates a throwing custom eventLogger internally, so it + // (and the internal `callback` below, which only calls resolve/reject + // and _logEvent()/_logError()) can't propagate an observer exception + // out of _flush() — that would skip clearing + // `_pending`/`_exposures`/`_goals` and, via `_finalize`'s callback never + // running, permanently strand `_finalizing` with no settlement. + this._logError( + new Error( + `Discarding ${this._exposures.length} exposures and ${this._goals.length} goals because context failed to initialize` + ) + ); this._pending = 0; this._exposures = []; this._goals = []; if (typeof callback === "function") { - try { - callback(); - } catch (observerError) { - console.error(observerError); - } + callback(); } return Promise.resolve(undefined); } @@ -1058,32 +1051,25 @@ export default class Context { // observe the publish attempt within the same tick, as before. const onFailure = (e: Error): Error => { this._pending += pendingCount; - this._exposures.push(...pendingExposures); - this._goals.push(...pendingGoals); + // Prepend rather than append: the restored batch failed to send while + // this._exposures/this._goals were already accumulating newer events + // recorded during the in-flight publish, so appending would put the + // older, previously-recorded events after the newer ones — reordering + // exposures/goals as seen by the collector. + this._exposures = pendingExposures.concat(this._exposures); + this._goals = pendingGoals.concat(this._goals); - try { - this._logError(e); - } catch (observerError) { - console.error(observerError); - } + this._logError(e); // Reschedule automatic delivery for the restored batch; this is a no-op // unless publishDelay >= 0 and no timer is already pending. this._setTimeout(); - // `callback` is internal glue (from `publish()`/`finalize()`), but it can - // itself invoke a user-supplied eventLogger (see `_finalize`'s callback, - // which calls `_logEvent("finalize")`). A throw there must not propagate - // into this promise chain: `_flushPromise` is cleared unconditionally - // below regardless of whether `onFailure`/`onSuccess` throw, but an - // unguarded throw here would still skip the `callback(e)` call's own - // completion and any code after it in the caller. + // `callback` is internal glue (from `publish()`/`finalize()`); it only + // calls resolve/reject and _logEvent()/_logError() (both of which + // isolate a throwing custom eventLogger internally), so it can't throw. if (typeof callback === "function") { - try { - callback(e); - } catch (observerError) { - console.error(observerError); - } + callback(e); } return e; @@ -1092,21 +1078,14 @@ export default class Context { // The batch has already been delivered by this point, so an exception from // observing that success (e.g. a throwing custom eventLogger) must not be // treated as a publish failure — that would incorrectly restore and resend - // an already-delivered batch. `callback()` still runs unconditionally - // afterward so `publish()`/`finalize()`'s own promise settles either way. + // an already-delivered batch. `_logEvent()` isolates the observer exception + // internally, and `callback()` still runs unconditionally afterward so + // `publish()`/`finalize()`'s own promise settles either way. const onSuccess = (): undefined => { - try { - this._logEvent("publish", request); - } catch (observerError) { - console.error(observerError); - } + this._logEvent("publish", request); if (typeof callback === "function") { - try { - callback(); - } catch (observerError) { - console.error(observerError); - } + callback(); } return undefined; @@ -1170,13 +1149,28 @@ export default class Context { private _logEvent(eventName: EventName, data?: Record) { if (this._eventLogger) { - this._eventLogger(this, eventName, data); + // A throwing custom eventLogger must never propagate out of this method: + // every call site treats logging as a side effect, and letting an + // observer exception escape here has repeatedly corrupted unrelated + // control flow (e.g. turning a successful init into a "failed" one when + // this call sits inside a .then() immediately followed by .catch(), + // or stranding a promise whose settlement was supposed to happen right + // after this call). + try { + this._eventLogger(this, eventName, data); + } catch (observerError) { + console.error(observerError); + } } } private _logError(error: Error) { if (this._eventLogger) { - this._eventLogger(this, "error", error); + try { + this._eventLogger(this, "error", error); + } catch (observerError) { + console.error(observerError); + } } } @@ -1276,12 +1270,7 @@ export default class Context { // fall through to `_flush`, which itself waits for any in-flight attempt. if (this._pending === 0 && !this._flushPromise) { this._finalized = true; - - try { - this._logEvent("finalize"); - } catch (observerError) { - console.error(observerError); - } + this._logEvent("finalize"); return Promise.resolve(); } @@ -1318,17 +1307,13 @@ export default class Context { } else { this._finalized = true; - // Settle the finalize promise before logging the event: a throwing - // custom eventLogger must not prevent `finalizing` from resolving - // (which would strand `isFinalizing()`/`isFinalized()` and any - // awaiters forever). + // Settle the finalize promise before logging the event: even though + // `_logEvent()` isolates a throwing custom eventLogger internally + // (it never throws), resolving first means `finalizing` settles + // exactly when the state it reflects (`_finalized`/`isFinalizing()`) + // becomes true, rather than depending on the logger call completing. resolveFinalizing(); - - try { - this._logEvent("finalize"); - } catch (observerError) { - console.error(observerError); - } + this._logEvent("finalize"); } }, requestOptions); From f01adb9d6cbfeb22e7f50f36221b85871aea42af Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 11:17:52 +0100 Subject: [PATCH 20/25] fix: normalize non-Promise publisher results; fix AbortSignal.reason parity in the shim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - context.ts: _flush() typed the custom publisher's return value as Promise but never verified it — a beacon-style publisher returning a bare boolean (or one that simply forgets to return its promise) made the `.then(onSuccess, onFailure)` access throw synchronously, outside the surrounding try/catch. This bricked the flush: the queue had already been cleared, but neither the callback nor the restore logic ran, so finalize() rejected with "publishResult.then is not a function" and isFinalizing() stayed true forever with no way to recover. Wrapped the publish() call in Promise.resolve(...) so any return value (or a synchronous throw) is normalized into the existing guarded try/catch and restore/reject path. - abort-controller-shim.ts: abort() didn't match native AbortController semantics in two ways — `??` replaced an explicit `null` reason with the default Error (native preserves null), and a second abort() call overwrote the latched reason and dispatched a second "abort" event (native ignores all calls after the first). Added an early return when already aborted, and switched to an `=== undefined` check so only an omitted/undefined reason gets the default error. - Added regression tests for both: a non-Promise-returning custom publisher through finalize(), and abort(null)/double-abort() shim parity. Both reproduced the reported bugs against the pre-fix code. --- src/__tests__/abort-controller-shim.test.js | 20 +++++++++++++++++ src/__tests__/context.test.js | 24 +++++++++++++++++++++ src/abort-controller-shim.ts | 11 +++++++++- src/context.ts | 9 +++++++- 4 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/__tests__/abort-controller-shim.test.js b/src/__tests__/abort-controller-shim.test.js index cc227f5..aa1680e 100644 --- a/src/__tests__/abort-controller-shim.test.js +++ b/src/__tests__/abort-controller-shim.test.js @@ -109,6 +109,26 @@ describe("AbortController", () => { const controller = new AbortController(); expect(controller.signal.reason).toBeUndefined(); }); + + it("should preserve an explicit null reason instead of falling back to the default error", () => { + // Matches native AbortController: only an omitted/undefined reason + // gets the default error; an explicit null is preserved as-is. + const controller = new AbortController(); + controller.abort(null); + expect(controller.signal.reason).toBeNull(); + }); + + it("should latch the first reason and ignore a second abort() call", () => { + const controller = new AbortController(); + const handler = jest.fn(); + controller.signal.addEventListener("abort", handler); + + controller.abort("first"); + controller.abort("second"); + + expect(controller.signal.reason).toBe("first"); + expect(handler).toHaveBeenCalledTimes(1); + }); }); describe("dispatchEvent onabort handling", () => { diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 3644e33..326e053 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4606,6 +4606,30 @@ describe("Context", () => { }); }); + it("should not brick finalize() when a custom publisher returns a non-Promise value", (done) => { + const context = new Context( + sdk, + { ...contextOptions, publishDelay: -1, refreshPeriod: 0 }, + contextParams, + getContextResponse + ); + + context.treatment("exp_test_ab"); + expect(context.pending()).toEqual(1); + + // A beacon-style or misimplemented custom publisher that forgets to + // return a promise (e.g. `navigator.sendBeacon`-style success flag). + publisher.publish.mockReturnValue(true); + + context.finalize().then(() => { + expect(context.pending()).toEqual(0); + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(true); + + done(); + }); + }); + it("should restore a failed batch ahead of events recorded during the in-flight publish, preserving chronological order", (done) => { const context = new Context( sdk, diff --git a/src/abort-controller-shim.ts b/src/abort-controller-shim.ts index af18593..e41c780 100644 --- a/src/abort-controller-shim.ts +++ b/src/abort-controller-shim.ts @@ -57,6 +57,15 @@ export class AbortController { signal = new AbortSignal(); abort(reason?: unknown) { + // Match native AbortController: a second call is a no-op (the first + // reason is latched, and the "abort" event fires at most once), and an + // explicit `null` reason is preserved as-is — only an omitted/undefined + // reason falls back to the default error. `??` would incorrectly replace + // an explicit `null` with the default. + if (this.signal.aborted) { + return; + } + let evt: Event | { type: string; bubbles: boolean; cancelable: boolean }; try { evt = new Event("abort"); @@ -69,7 +78,7 @@ export class AbortController { } this.signal.aborted = true; - this.signal.reason = reason ?? new Error("The operation was aborted."); + this.signal.reason = reason === undefined ? new Error("The operation was aborted.") : reason; this.signal.dispatchEvent(evt); } diff --git a/src/context.ts b/src/context.ts index a22e043..f9cfb32 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1093,7 +1093,14 @@ export default class Context { let publishResult: Promise; try { - publishResult = this._publisher.publish(request, this._sdk, this, requestOptions); + // `Promise.resolve(...)` normalizes the extension point: a custom + // publisher is only required to conform to `ContextPublisher`'s type at + // compile time, but nothing stops a runtime implementation from + // returning a non-Promise (e.g. a bare boolean from a beacon-style + // publisher, or a forgotten `return`). Without normalizing, the `.then` + // access below would throw synchronously outside this `try`, bricking + // the flush before the queue could be restored or the callback invoked. + publishResult = Promise.resolve(this._publisher.publish(request, this._sdk, this, requestOptions)); } catch (e) { const result = onFailure(e as Error); this._flushPromise = undefined; From 461f6cf2f6aa7f9db400d10e8685f9764e56294e Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 11:29:23 +0100 Subject: [PATCH 21/25] chore: keep this as a minor release (1.14.0) instead of bumping to 2.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review discussion, this release stays 1.x — 1.14.0 finalizes the 1.14.0-beta.1 that was already in progress on this branch, rather than a major 2.0.0 bump. Updated the README's migration guide heading and version reference to match; src/version.ts (gitignored, generated from package.json by scripts/generate-version.js) was regenerated locally and picked up by the test suite automatically. --- README.md | 6 +++--- package.json | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 76506fc..d064bf2 100644 --- a/README.md +++ b/README.md @@ -507,9 +507,9 @@ document.getElementById("checkout-btn").addEventListener("click", () => { }); ``` -## Migration Guide (v1 → v2) +## Migration Guide (1.13.x → 1.14.0) -Version 2.0.0 contains breaking changes made for cross-SDK consistency and correctness. These align the JavaScript SDK with the Python, Swift, Java, and other A/B Smartly SDKs. Most applications will not need code changes, but review the items below. +Version 1.14.0 contains breaking changes made for cross-SDK consistency and correctness. These align the JavaScript SDK with the Python, Swift, Java, and other A/B Smartly SDKs. Most applications will not need code changes, but review the items below. ### `ready()` no longer resolves with the Error object on failure @@ -533,7 +533,7 @@ const variant = context.treatment("exp_test"); // returns 0 (control) on failure **After:** Unit IDs are encoded as canonical 4-byte UTF-8, matching the A/B Smartly collector (which hashes with `UTF_8`) and the SDKs already using native UTF-8 (Go, Python, Ruby, etc.). -**When this might be a problem:** A unit ID that contains an astral character (emoji, rare CJK, etc.) may now be assigned a **different variant** than it was under v1. **Unit IDs composed entirely of BMP characters (≤ U+FFFF) — which covers essentially all typical session IDs, UUIDs, and user IDs — are unaffected.** This only changes assignment for units whose IDs contain astral characters. +**When this might be a problem:** A unit ID that contains an astral character (emoji, rare CJK, etc.) may now be assigned a **different variant** than it was in earlier versions. **Unit IDs composed entirely of BMP characters (≤ U+FFFF) — which covers essentially all typical session IDs, UUIDs, and user IDs — are unaffected.** This only changes assignment for units whose IDs contain astral characters. ### `audienceMismatch` cache invalidation on indeterminate audiences diff --git a/package.json b/package.json index 1f074df..4739452 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@absmartly/javascript-sdk", - "version": "2.0.0", + "version": "1.14.0", "description": "A/B Smartly Javascript SDK", "homepage": "https://github.com/absmartly/javascript-sdk#README.md", "bugs": "https://github.com/absmartly/javascript-sdk/issues", From 7c92faf674a07804f1e84b75c69050e72942ac68 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 11:31:07 +0100 Subject: [PATCH 22/25] docs: drop cross-SDK framing from the migration guide Per Cal's feedback: keep the migration guide as a general changelog entry rather than framing it as cross-SDK parity work, and don't list sibling-language SDKs by name. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d064bf2..013ce08 100644 --- a/README.md +++ b/README.md @@ -509,7 +509,7 @@ document.getElementById("checkout-btn").addEventListener("click", () => { ## Migration Guide (1.13.x → 1.14.0) -Version 1.14.0 contains breaking changes made for cross-SDK consistency and correctness. These align the JavaScript SDK with the Python, Swift, Java, and other A/B Smartly SDKs. Most applications will not need code changes, but review the items below. +Version 1.14.0 contains a few breaking changes. Most applications will not need code changes, but review the items below. ### `ready()` no longer resolves with the Error object on failure @@ -531,7 +531,7 @@ const variant = context.treatment("exp_test"); // returns 0 (control) on failure **Before:** `stringToUint8Array` (used to hash unit IDs for variant assignment) encoded each UTF-16 code unit independently. A character outside the Basic Multilingual Plane (≥ U+10000, e.g. an emoji) was encoded as an invalid CESU-8 byte sequence rather than canonical UTF-8. -**After:** Unit IDs are encoded as canonical 4-byte UTF-8, matching the A/B Smartly collector (which hashes with `UTF_8`) and the SDKs already using native UTF-8 (Go, Python, Ruby, etc.). +**After:** Unit IDs are encoded as canonical 4-byte UTF-8, matching the A/B Smartly collector (which hashes with `UTF_8`). **When this might be a problem:** A unit ID that contains an astral character (emoji, rare CJK, etc.) may now be assigned a **different variant** than it was in earlier versions. **Unit IDs composed entirely of BMP characters (≤ U+FFFF) — which covers essentially all typical session IDs, UUIDs, and user IDs — are unaffected.** This only changes assignment for units whose IDs contain astral characters. From c8ed78798bf82a7abcc9b02e54ab11567a398fe7 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 14:05:46 +0100 Subject: [PATCH 23/25] fix: normalize falsy publish rejection reasons; use AbortError-named default reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer-reported issues: - A publisher rejecting with a falsy reason (undefined/null/false) was passed straight to publish()/finalize() callbacks, which decide success with `if (error)` — silently treating the failed publish as success and leaving the restored batch unretryable. Falsy reasons are now normalized to a truthy Error before reaching the callback. - The abort-controller-shim's default abort reason was a plain Error named "Error", unlike native AbortController which uses a DOMException named "AbortError". Callers that classify cancellation via signal.reason.name now see the same behavior as native. --- src/__tests__/abort-controller-shim.test.js | 2 +- src/__tests__/context.test.js | 44 +++++++++++++++++++++ src/abort-controller-shim.ts | 15 ++++++- src/context.ts | 31 ++++++++++++--- 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/__tests__/abort-controller-shim.test.js b/src/__tests__/abort-controller-shim.test.js index aa1680e..03f3a48 100644 --- a/src/__tests__/abort-controller-shim.test.js +++ b/src/__tests__/abort-controller-shim.test.js @@ -94,8 +94,8 @@ describe("AbortController", () => { const controller = new AbortController(); controller.abort(); expect(controller.signal.aborted).toBe(true); - expect(controller.signal.reason).toBeInstanceOf(Error); expect(controller.signal.reason.message).toBe("The operation was aborted."); + expect(controller.signal.reason.name).toBe("AbortError"); }); it("should set custom reason on abort(reason)", () => { diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 326e053..55896a4 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -3679,6 +3679,28 @@ describe("Context", () => { }); }); + it.each([undefined, null, false])( + "should reject and restore the batch when the publisher rejects with the falsy reason %p", + (falsyReason, done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + context.track("goal1", { amount: 125 }); + expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.reject(falsyReason)); + + context.publish().then( + () => done(new Error("publish() must not resolve when the publisher rejected")), + (e) => { + expect(e).toBeTruthy(); + expect(context.pending()).toEqual(1); + + done(); + } + ); + } + ); + it("should call client publish", (done) => { const context = new Context(sdk, contextOptions, contextParams, getContextResponse); @@ -4368,6 +4390,28 @@ describe("Context", () => { expect(context.isFinalized()).toEqual(false); }); + it.each([undefined, null, false])( + "should reject and leave finalize() unfinalized when the publisher rejects with the falsy reason %p", + (falsyReason, done) => { + const context = new Context(sdk, contextOptions, contextParams, getContextResponse); + + context.treatment("exp_test_ab"); + + publisher.publish.mockReturnValue(Promise.reject(falsyReason)); + + context.finalize().then( + () => done(new Error("finalize() must not resolve when the publisher rejected")), + (e) => { + expect(e).toBeTruthy(); + expect(context.isFinalizing()).toEqual(false); + expect(context.isFinalized()).toEqual(false); + + done(); + } + ); + } + ); + it("should call event logger on success", (done) => { const context = new Context(sdk, contextOptions, contextParams, getContextResponse); diff --git a/src/abort-controller-shim.ts b/src/abort-controller-shim.ts index e41c780..17d9c68 100644 --- a/src/abort-controller-shim.ts +++ b/src/abort-controller-shim.ts @@ -52,6 +52,19 @@ export class AbortSignal { } } +// Native AbortController defaults `signal.reason` to a `DOMException` named +// "AbortError" (not a plain `Error`), and callers classify cancellation via +// `signal.reason.name`. `DOMException` isn't guaranteed to exist in the older +// environments this shim targets, so fall back to a same-named `Error`. +function createDefaultAbortReason(): Error { + if (typeof DOMException !== "undefined") { + return new DOMException("The operation was aborted.", "AbortError") as unknown as Error; + } + const error = new Error("The operation was aborted."); + error.name = "AbortError"; + return error; +} + // eslint-disable-next-line no-shadow export class AbortController { signal = new AbortSignal(); @@ -78,7 +91,7 @@ export class AbortController { } this.signal.aborted = true; - this.signal.reason = reason === undefined ? new Error("The operation was aborted.") : reason; + this.signal.reason = reason === undefined ? createDefaultAbortReason() : reason; this.signal.dispatchEvent(evt); } diff --git a/src/context.ts b/src/context.ts index f9cfb32..9985120 100644 --- a/src/context.ts +++ b/src/context.ts @@ -932,6 +932,18 @@ export default class Context { return allAttributes; } + // A caught exception or a rejected promise's reason can be any value. Only + // a truthy reason survives the `if (error)` checks in `publish()`'s and + // `finalize()`'s callbacks, so a falsy one (`undefined`, `null`, `false`) + // must be replaced with a truthy `Error`; a truthy non-Error reason (e.g. a + // plain string) is left as-is to preserve its original shape. + private _asFailure(reason: unknown): Error { + if (reason) { + return reason as Error; + } + return new Error(`Publish failed with a falsy reason: ${String(reason)}`); + } + private _flush( callback?: (error?: Error) => void, requestOptions?: ClientRequestOptions @@ -1021,12 +1033,13 @@ export default class Context { request.attributes = allAttributes; } } catch (e) { - this._logError(e as Error); + const failure = this._asFailure(e); + this._logError(failure); if (typeof callback === "function") { - callback(e as Error); + callback(failure); } - return Promise.resolve(e as Error); + return Promise.resolve(failure); } // Snapshot and reset synchronously before the async publish. @@ -1049,7 +1062,15 @@ export default class Context { // as an async rejection, so the snapshot is restored either way. The call // itself stays synchronous (no extra microtask hop) so timer-driven callers // observe the publish attempt within the same tick, as before. - const onFailure = (e: Error): Error => { + const onFailure = (reason: unknown): Error => { + // A custom publisher can reject with any value, including a falsy one + // (`undefined`, `null`, `false`). `publish()`/`finalize()` decide success + // with `if (error)`, so a falsy reason would otherwise be mistaken for a + // successful publish while this restored batch is left stranded. Normalize + // only falsy reasons to a truthy `Error`; a truthy non-Error reason (e.g. a + // plain string) is passed through unchanged to preserve its original shape. + const e = this._asFailure(reason); + this._pending += pendingCount; // Prepend rather than append: the restored batch failed to send while // this._exposures/this._goals were already accumulating newer events @@ -1102,7 +1123,7 @@ export default class Context { // the flush before the queue could be restored or the callback invoked. publishResult = Promise.resolve(this._publisher.publish(request, this._sdk, this, requestOptions)); } catch (e) { - const result = onFailure(e as Error); + const result = onFailure(e); this._flushPromise = undefined; return Promise.resolve(result); } From 6b565a1475220463f08f23fc8bd37e8576065c77 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Sat, 12 Sep 2026 09:18:27 +0100 Subject: [PATCH 24/25] fix: guard DOMException construction for legacy non-constructible globals IE 10 (the browser build's floor) exposes DOMException as a global but throws on `new DOMException(...)`. A typeof check alone selected that branch and let the construction exception propagate out of abort(), leaving signal.reason unset and no abort event dispatched. Wrap the construction itself so it falls back to the Error-based reason. --- src/__tests__/abort-controller-shim.test.js | 21 +++++++++++++++++++++ src/abort-controller-shim.ts | 12 ++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/__tests__/abort-controller-shim.test.js b/src/__tests__/abort-controller-shim.test.js index 03f3a48..319a64b 100644 --- a/src/__tests__/abort-controller-shim.test.js +++ b/src/__tests__/abort-controller-shim.test.js @@ -98,6 +98,27 @@ describe("AbortController", () => { expect(controller.signal.reason.name).toBe("AbortError"); }); + it("should fall back to an Error-based reason when DOMException is defined but not constructible", () => { + // Matches legacy environments (e.g. IE 10, this shim's floor) that expose + // DOMException as a global but throw on `new DOMException(...)`. + const originalDOMException = global.DOMException; + global.DOMException = function NonConstructibleDOMException() { + throw new TypeError("Illegal constructor"); + }; + + try { + const controller = new AbortController(); + controller.abort(); + + expect(controller.signal.aborted).toBe(true); + expect(controller.signal.reason).toBeInstanceOf(Error); + expect(controller.signal.reason.message).toBe("The operation was aborted."); + expect(controller.signal.reason.name).toBe("AbortError"); + } finally { + global.DOMException = originalDOMException; + } + }); + it("should set custom reason on abort(reason)", () => { const controller = new AbortController(); const customReason = new Error("custom abort"); diff --git a/src/abort-controller-shim.ts b/src/abort-controller-shim.ts index 17d9c68..2e84c30 100644 --- a/src/abort-controller-shim.ts +++ b/src/abort-controller-shim.ts @@ -55,10 +55,18 @@ export class AbortSignal { // Native AbortController defaults `signal.reason` to a `DOMException` named // "AbortError" (not a plain `Error`), and callers classify cancellation via // `signal.reason.name`. `DOMException` isn't guaranteed to exist in the older -// environments this shim targets, so fall back to a same-named `Error`. +// environments this shim targets, so fall back to a same-named `Error`. Some +// legacy environments (e.g. IE 10, the browser build's floor) expose +// `DOMException` as a global but don't support constructing it with `new` — +// a `typeof` check alone would select this branch and then throw, so the +// construction itself must be guarded too. function createDefaultAbortReason(): Error { if (typeof DOMException !== "undefined") { - return new DOMException("The operation was aborted.", "AbortError") as unknown as Error; + try { + return new DOMException("The operation was aborted.", "AbortError") as unknown as Error; + } catch (error) { + // Fall through to the Error-based fallback below. + } } const error = new Error("The operation was aborted."); error.name = "AbortError"; From 47d6d9a7f658b1e0a7a72ae3c1acb6d711caa9b3 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Mon, 21 Sep 2026 16:29:32 +0100 Subject: [PATCH 25/25] fix: prevent automatic publish retry loops Preserve failed batches for a later explicit flush while avoiding timer-driven retry storms, and ensure TextEncoder views are copied before hashing. --- package-lock.json | 4 ++-- src/__tests__/context.test.js | 25 ++++++++++++------------- src/__tests__/utils.test.js | 29 +++++++++++++++++++++++++++++ src/context.ts | 6 +----- src/utils.ts | 2 +- 5 files changed, 45 insertions(+), 21 deletions(-) diff --git a/package-lock.json b/package-lock.json index d921a01..171d69f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@absmartly/javascript-sdk", - "version": "2.0.0", + "version": "1.14.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@absmartly/javascript-sdk", - "version": "2.0.0", + "version": "1.14.0", "license": "Apache-2.0", "dependencies": { "@babel/runtime": "^7.29.2", diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 55896a4..4dacff5 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4830,7 +4830,7 @@ describe("Context", () => { }); }); - it("should reschedule the automatic publish timer after a scheduled flush fails", (done) => { + it("should keep a failed scheduled batch pending until another event schedules a flush", (done) => { jest.useFakeTimers("legacy"); jest.spyOn(global, "setTimeout"); @@ -4850,28 +4850,27 @@ describe("Context", () => { jest.advanceTimersByTime(publishDelay); - // Flush the microtask queue so the rejection handler (which restores the - // queue and reschedules) has run before we assert on it. Promise.resolve() .then(() => Promise.resolve()) .then(() => { expect(context.pending()).toEqual(1); - // A new automatic-publish timer must have been scheduled for the - // restored batch, otherwise it is silently dropped forever. + expect(setTimeout).toHaveBeenCalledTimes(1); + expect(publisher.publish).toHaveBeenCalledTimes(1); + + context.track("goal1"); + expect(context.pending()).toEqual(2); expect(setTimeout).toHaveBeenCalledTimes(2); publisher.publish.mockReturnValueOnce(Promise.resolve()); - jest.advanceTimersByTime(publishDelay); - Promise.resolve() - .then(() => Promise.resolve()) - .then(() => { - expect(context.pending()).toEqual(0); - expect(publisher.publish).toHaveBeenCalledTimes(2); + return Promise.resolve().then(() => Promise.resolve()); + }) + .then(() => { + expect(context.pending()).toEqual(0); + expect(publisher.publish).toHaveBeenCalledTimes(2); - done(); - }); + done(); }); }); }); diff --git a/src/__tests__/utils.test.js b/src/__tests__/utils.test.js index 62aec2b..0331d0c 100644 --- a/src/__tests__/utils.test.js +++ b/src/__tests__/utils.test.js @@ -367,6 +367,35 @@ describe("stringToUint8Array()", () => { done(); }); + it("should copy bytes from a TextEncoder view with an oversized backing buffer", (done) => { + const OriginalTextEncoder = global.TextEncoder; + const expected = hashUnit("session_abc123"); + + // eslint-disable-next-line no-global-assign + global.TextEncoder = class { + encode(value) { + const bytes = OriginalTextEncoder + ? new OriginalTextEncoder().encode(value) + : Uint8Array.from([115, 101, 115, 115, 105, 111, 110, 95, 97, 98, 99, 49, 50, 51]); + const buffer = new ArrayBuffer(bytes.byteLength + 16); + const view = new Uint8Array(buffer, 8, bytes.byteLength); + view.set(bytes); + return view; + } + }; + + try { + const bytes = stringToUint8Array("session_abc123"); + expect(bytes.byteOffset).toBe(0); + expect(bytes.byteLength).toBe(bytes.buffer.byteLength); + expect(hashUnit("session_abc123")).toBe(expected); + } finally { + global.TextEncoder = OriginalTextEncoder; + } + + done(); + }); + it("should produce identical hashUnit results for both code paths", (done) => { const OriginalTextEncoder = global.TextEncoder; diff --git a/src/context.ts b/src/context.ts index 9985120..8267382 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1046,7 +1046,7 @@ export default class Context { // The data is already copied into `request` via .map(), so clearing // immediately is safe and allows new events to accumulate during the // in-flight publish. On failure, we restore the snapshot so the events - // are retried on the next flush cycle. `_flushPromise` tracks this in-flight + // remain pending for a later flush. `_flushPromise` tracks this in-flight // attempt so concurrent callers (e.g. `finalize()`) can wait on it instead of // racing the synchronous reset above. const pendingCount = this._pending; @@ -1082,10 +1082,6 @@ export default class Context { this._logError(e); - // Reschedule automatic delivery for the restored batch; this is a no-op - // unless publishDelay >= 0 and no timer is already pending. - this._setTimeout(); - // `callback` is internal glue (from `publish()`/`finalize()`); it only // calls resolve/reject and _logEvent()/_logError() (both of which // isolate a throwing custom eventLogger internally), so it can't throw. diff --git a/src/utils.ts b/src/utils.ts index a1671bb..2c6dd11 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -177,7 +177,7 @@ export function toWellFormedString(value: string): string { export function stringToUint8Array(value: string) { if (typeof TextEncoder !== "undefined") { - return new TextEncoder().encode(value); + return new Uint8Array(new TextEncoder().encode(value)); } const utf8: number[] = [];