Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a0956f5. Configure here.
|
|
||
| const convertedContext: LDContextCommon = { key: finalKey }; | ||
| Object.entries(inCommon).forEach(([key, value]) => { | ||
| if (key === 'targetingKey' || key === 'key') { |
There was a problem hiding this comment.
kind attribute leaks through spread, overriding finalKind
Medium Severity
translateContextCommon filters out targetingKey and key but not kind. For single-kind contexts, the kind attribute passes through convertAttributes and ends up in the returned object. Because the spread in { kind: finalKind, ...translateContextCommon(...) } comes second, the unfiltered kind overrides finalKind. This breaks the fallback to 'user' when kind is a non-string value — the invalid value leaks into the LDContext despite the warning at line 139. The multi-context path correctly filters kind at line 120, but translateContextCommon does not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a0956f5. Configure here.
There was a problem hiding this comment.
Existing bug... will track internally
| object[key] = {}; | ||
| Object.entries(value).forEach(([objectKey, objectValue]) => { | ||
| convertAttributes(logger, objectKey, objectValue, object[key], [...visited, value]); | ||
| }); |
There was a problem hiding this comment.
Null attribute values crash in convertAttributes
Medium Severity
In convertAttributes, when value is null, typeof null === 'object' is true and !Array.isArray(null) is true, so the code enters the object branch. It then calls Object.entries(null) on line 43, which throws an unhandled TypeError: Cannot convert undefined or null to object. Any custom attribute with a null value in the evaluation context (e.g., { targetingKey: 'key', myAttr: null }) will crash the provider. A null guard is needed before or within the typeof value === 'object' check.
Reviewed by Cursor Bugbot for commit a0956f5. Configure here.
There was a problem hiding this comment.
Existing bug... will track internally
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (key === 'targetingKey' || key === 'key') { | ||
| return; |
There was a problem hiding this comment.
🔴 kind attribute not filtered in translateContextCommon, overrides corrected finalKind via object spread
When a single-kind evaluation context has a non-string kind (e.g., { kind: 42, targetingKey: 'foo' }), translateContext correctly detects this at line 139, logs a warning, and keeps finalKind = 'user'. However, translateContextCommon does not skip kind in its attribute iteration (line 86 only skips targetingKey and key), so kind flows through convertAttributes and is set on the returned convertedContext object. Back in translateContext at line 143-145, the spread { kind: finalKind, ...translateContextCommon(...) } causes the non-string kind from the spread to override the corrected finalKind. The resulting LDContext has an invalid non-string kind (e.g., kind: 42 instead of kind: 'user'), which will cause incorrect behavior in the LD SDK's flag evaluation.
| if (key === 'targetingKey' || key === 'key') { | |
| return; | |
| if (key === 'targetingKey' || key === 'key' || key === 'kind') { | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| translateContext(this._logger, context), | ||
| defaultValue, | ||
| ); | ||
| if (typeof res.value === 'object') { |
There was a problem hiding this comment.
🟡 resolveObjectEvaluation treats null as a valid object due to typeof null === 'object'
At BaseOpenFeatureProvider.ts:174, typeof res.value === 'object' is used to check whether the LD SDK returned an object. In JavaScript, typeof null === 'object' evaluates to true, so if the SDK returns null (e.g., for a JSON flag whose value is null), the check passes and null is returned as a successful object resolution. The caller of resolveObjectEvaluation expects an object value but receives null, which violates the OpenFeature contract. The fix should add a null guard: res.value !== null && typeof res.value === 'object'.
| if (typeof res.value === 'object') { | |
| if (res.value !== null && typeof res.value === 'object') { | |
Was this helpful? React with 👍 or 👎 to provide feedback.
joker23
force-pushed
the
skz/sdk-2215/openfeature-server-common
branch
from
a0956f5 to
a076b4f
Compare
| } else if (typeof value === 'object' && !Array.isArray(value)) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| object[key] = {}; | ||
| Object.entries(value).forEach(([objectKey, objectValue]) => { | ||
| convertAttributes(logger, objectKey, objectValue, object[key], [...visited, value]); |
There was a problem hiding this comment.
🔴 convertAttributes crashes with TypeError when attribute value is null
typeof null === 'object' is true in JavaScript, and !Array.isArray(null) is also true. When a custom attribute has a null value (which is a valid EvaluationContextValue — it's included in OpenFeature's PrimitiveValue type), the code at line 40 enters the object branch and calls Object.entries(null) at line 43, which throws a TypeError: Cannot convert undefined or null to object. For example, translateContext(logger, { targetingKey: 'key', someAttr: null }) will crash. The same typeof null === 'object' issue also affects the multi-context path at translateContext.ts:122 — a null sub-context value would enter the object branch and crash when accessing valueRecord.targetingKey.
| } else if (typeof value === 'object' && !Array.isArray(value)) { | |
| // eslint-disable-next-line no-param-reassign | |
| object[key] = {}; | |
| Object.entries(value).forEach(([objectKey, objectValue]) => { | |
| convertAttributes(logger, objectKey, objectValue, object[key], [...visited, value]); | |
| } else if (value != null && typeof value === 'object' && !Array.isArray(value)) { | |
| // eslint-disable-next-line no-param-reassign | |
| object[key] = {}; | |
| Object.entries(value).forEach(([objectKey, objectValue]) => { | |
| convertAttributes(logger, objectKey, objectValue, object[key], [...visited, value]); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR will initialize the migration of openfeature node server provider to js-core. Specifically, this PR will create a shared openfeature server provider base class.
REVIEWER: The logic in this should be a more generalized version of https://github.com/launchdarkly/openfeature-node-server/tree/main.
Theoretically, this would allow us to implement openfeature providers pretty easily (eg https://github.com/launchdarkly/js-core/blob/skz/sdk-2213/openfeature-migration/packages/sdk/openfeature-node-server/src/LaunchDarklyProvider.ts)
Note
Medium Risk
Adds a new provider foundation layer that will be reused by future SDKs; translation/typing logic and context handling bugs could affect flag evaluation and tracking once adopted.
Overview
Introduces a new shared workspace package,
@launchdarkly/openfeature-js-server-common, to centralize LaunchDarkly’s server-side OpenFeature provider implementation.The package adds
BaseOpenFeatureProvider(initialization, typedresolve*Evaluationmethods, OpenFeature events, andtrackbridging) plus utilities to translate OpenFeature evaluation context/results/tracking details into LaunchDarkly equivalents, with cycle detection and attribute validation covered by new Jest tests. Repo plumbing is updated to include the workspace, TypeScript project reference, andrelease-pleaseconfiguration for pre-1.0 versioning.Reviewed by Cursor Bugbot for commit a076b4f. Bugbot is set up for automated code reviews on this repo. Configure here.