Skip to content

Conversation

@joker23
Copy link
Contributor

@joker23 joker23 commented Dec 10, 2025

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

sdk-1653
sdk-1565

Describe the solution you've provided

  • Added LDDebugOverride interface to manage flag value overrides during development.
  • Introduced safeRegisterDebugOverridePlugins function to register plugins with debug capabilities.
  • Updated FlagManager to support debug overrides, including methods to set, remove, and clear overrides.
  • Enhanced LDClientImpl to utilize debug overrides during client initialization.
  • Refactored LDPlugin interface to include optional registerDebug method for plugins.

This PR will enable @launchdarkly/toolbar to use 4.x

Additional context

  • This PR is based off of feat: implement waitForInitialization for browser sdk 4.x #1028 (will need to do some rebase magic later)
  • There are a few places that I've marked REVIEWER that I would like some additional discussions before merging
  • In this change we are assuming that the plugin support (eg registering plugins) is still the responsibility of individual SDK implementations.

Note

Introduces a debug flag override interface, wires plugin registration for overrides, and updates flag management and clients to support and emit override-driven changes.

  • Flag management:
    • Add LDDebugOverride interface and implement in DefaultFlagManager (setOverride, removeOverride, clearAllOverrides, getAllOverrides).
    • get/getAll now merge in-memory overrides with stored flags; override values take precedence.
    • Emit change callbacks with type "override" via FlagUpdater.handleFlagChanges.
  • Updater:
    • Extend FlagChangeType with "override"; add handleFlagChanges helper; refactor init/upsert to use it.
  • Client integration:
    • LDClientImpl: expose getDebugOverrides() from FlagManager and continue inspection updates on changes.
    • Browser client: during registerPlugins, call safeRegisterDebugOverridePlugins to pass overrides to plugins.
  • Plugin API & utilities:
    • Add LDPluginBase.registerDebug(debugOverride) optional method.
    • New safeRegisterDebugOverridePlugins utility to safely register debug override plugins.
  • Exports:
    • Re-export LDPluginBase, LDDebugOverride, and safeRegisterDebugOverridePlugins in public entrypoints.
  • Tests:
    • Add comprehensive FlagManager override tests covering precedence, callbacks, clearing, and merging.

Written by Cursor Bugbot for commit 2bf10a3. This will update automatically on new commits. Configure here.

@github-actions
Copy link
Contributor

@launchdarkly/browser size report
This is the brotli compressed size of the ESM build.
Compressed size: 169118 bytes
Compressed size limit: 200000
Uncompressed size: 789399 bytes

@github-actions
Copy link
Contributor

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 25394 bytes
Compressed size limit: 26000
Uncompressed size: 124693 bytes

@github-actions
Copy link
Contributor

github-actions bot commented Dec 10, 2025

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 18527 bytes
Compressed size limit: 20000
Uncompressed size: 95226 bytes

@github-actions
Copy link
Contributor

github-actions bot commented Dec 10, 2025

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Compressed size: 22955 bytes
Compressed size limit: 25000
Uncompressed size: 79621 bytes

@joker23 joker23 changed the base branch from main to skz/sdk-1360/browser-4.x-waitforinit December 10, 2025 19:32
@joker23 joker23 changed the base branch from skz/sdk-1360/browser-4.x-waitforinit to main December 10, 2025 19:32
@joker23 joker23 force-pushed the skz/sdk-1653/add-debug-override branch from d13c429 to 1fd0057 Compare December 15, 2025 19:55
- Added `LDDebugOverride` interface to manage flag value overrides during development.
- Introduced `safeRegisterDebugOverridePlugins` function to register plugins with debug capabilities.
- Updated `FlagManager` to support debug overrides, including methods to set, remove, and clear overrides.
- Enhanced `LDClientImpl` to utilize debug overrides during client initialization.
- Refactored `LDPlugin` interface to include optional `registerDebug` method for plugins.

This commit will enable `@launchdarkly/toolbar` to use 4.x
@joker23 joker23 force-pushed the skz/sdk-1653/add-debug-override branch 2 times, most recently from 8b6f8d9 to cfd87be Compare December 15, 2025 22:32
@joker23 joker23 marked this pull request as ready for review December 15, 2025 23:37
@joker23 joker23 requested a review from a team as a code owner December 15, 2025 23:37
@joker23 joker23 requested a review from kinyoklion December 15, 2025 23:37
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Inspection manager not notified for override flag changes

The _handleInspectionChanged method only handles 'init' and 'patch' flag change types, but the PR introduces a new 'override' type. When override changes occur (via setOverride, removeOverride, or clearAllOverrides), the method computes the details object but never sends it to the inspector manager because there's no branch handling type === 'override'. This means inspectors will not be notified when debug overrides change flag values.

packages/shared/sdk-client/src/LDClientImpl.ts#L638-L645

});
if (type === 'init') {
this._inspectorManager.onFlagsChanged(details);
} else if (type === 'patch') {
Object.entries(details).forEach(([flagKey, detail]) => {
this._inspectorManager.onFlagChanged(flagKey, detail);
});
}

Fix in Cursor Fix in Web


@joker23
Copy link
Contributor Author

joker23 commented Dec 16, 2025

@kinyoklion would we want to treat overrides the same as normal flag changes? My thoughts are no since the debug override use cases so far has mostly been for development. Or perhaps we can create another inspector type for these events?

@joker23 joker23 force-pushed the skz/sdk-1653/add-debug-override branch from cfd87be to b8f8d9a Compare December 16, 2025 16:09
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Inspector notifications silently skipped for override changes

The _handleInspectionChanged method only handles 'init' and 'patch' flag change types with explicit if/else if conditions, but the newly added 'override' type falls through without any handling. This causes inspector notifications (onFlagsChanged and onFlagChanged) to be silently skipped when flag overrides change, while the regular change events are still emitted via the emitter. This inconsistency means inspector hooks won't be notified of override changes.

packages/shared/sdk-client/src/LDClientImpl.ts#L634-L641

});
if (type === 'init') {
this._inspectorManager.onFlagsChanged(details);
} else if (type === 'patch') {
Object.entries(details).forEach(([flagKey, detail]) => {
this._inspectorManager.onFlagChanged(flagKey, detail);
});
}

packages/shared/sdk-client/src/flag-manager/FlagUpdater.ts#L6-L7

export type FlagChangeType = 'init' | 'patch' | 'override';

Fix in Cursor Fix in Web


/**
* Safe register debug override plugins.
*
* @param logger - The logger to use for logging errors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We don't need the hyphens. Maybe leftover from the jsdoc.

if (this._overrides) {
const clearedOverrides = { ...this._overrides };
this._overrides = undefined; // Reset to undefined
this._flagUpdater.handleFlagChanges(Object.keys(clearedOverrides), 'override');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat.

@kinyoklion
Copy link
Member

Bug: Inspector notifications silently skipped for override changes

The _handleInspectionChanged method only handles 'init' and 'patch' flag change types with explicit if/else if conditions, but the newly added 'override' type falls through without any handling. This causes inspector notifications (onFlagsChanged and onFlagChanged) to be silently skipped when flag overrides change, while the regular change events are still emitted via the emitter. This inconsistency means inspector hooks won't be notified of override changes.

packages/shared/sdk-client/src/LDClientImpl.ts#L634-L641

packages/shared/sdk-client/src/flag-manager/FlagUpdater.ts#L6-L7

Fix in Cursor Fix in Web

This sounds like the desired behavior? I don't think we would want to inform them of debug activity. But maybe we would want an explicit comment.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: New 'override' FlagChangeType not handled in inspection callback

The new 'override' value is added to FlagChangeType, but _handleInspectionChanged only handles 'init' and 'patch' types. When override changes occur, the function builds the details object (lines 619-633) but then neither conditional branch (line 635-641) applies, so the details object is discarded without being used. This causes unnecessary work when inspectors exist and override changes happen. The function needs to either return early when type === 'override' to avoid the wasted computation, or explicitly handle the 'override' case if inspectors should be notified of override changes.

packages/shared/sdk-client/src/LDClientImpl.ts#L613-L642

private _handleInspectionChanged(flagKeys: Array<string>, type: FlagChangeType) {
if (!this._inspectorManager.hasInspectors()) {
return;
}
const details: Record<string, LDEvaluationDetail> = {};
flagKeys.forEach((flagKey) => {
const item = this._flagManager.get(flagKey);
if (item?.flag && !item.flag.deleted) {
const { reason, value, variation } = item.flag;
details[flagKey] = createSuccessEvaluationDetail(value, variation, reason);
} else {
details[flagKey] = {
value: undefined,
// For backwards compatibility purposes reason and variationIndex are null instead of
// being undefined.
reason: null,
variationIndex: null,
};
}
});
if (type === 'init') {
this._inspectorManager.onFlagsChanged(details);
} else if (type === 'patch') {
Object.entries(details).forEach(([flagKey, detail]) => {
this._inspectorManager.onFlagChanged(flagKey, detail);
});
}
}

packages/shared/sdk-client/src/flag-manager/FlagUpdater.ts#L6-L7

export type FlagChangeType = 'init' | 'patch' | 'override';

Fix in Cursor Fix in Web


@joker23 joker23 merged commit 17f5e7d into main Dec 17, 2025
37 checks passed
@joker23 joker23 deleted the skz/sdk-1653/add-debug-override branch December 17, 2025 00:29
@github-actions github-actions bot mentioned this pull request Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants