-
Notifications
You must be signed in to change notification settings - Fork 31
feat: adding support for debug override plugins #1033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
d13c429 to
1fd0057
Compare
- 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
8b6f8d9 to
cfd87be
Compare
There was a problem hiding this 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
js-core/packages/shared/sdk-client/src/LDClientImpl.ts
Lines 638 to 645 in cfd87be
| }); | |
| if (type === 'init') { | |
| this._inspectorManager.onFlagsChanged(details); | |
| } else if (type === 'patch') { | |
| Object.entries(details).forEach(([flagKey, detail]) => { | |
| this._inspectorManager.onFlagChanged(flagKey, detail); | |
| }); | |
| } |
|
@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? |
cfd87be to
b8f8d9a
Compare
There was a problem hiding this 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
js-core/packages/shared/sdk-client/src/LDClientImpl.ts
Lines 634 to 641 in b8f8d9a
| }); | |
| 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'; |
| /** | ||
| * Safe register debug override plugins. | ||
| * | ||
| * @param logger - The logger to use for logging errors. |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat.
Co-authored-by: Ryan Lamb <[email protected]>
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. |
There was a problem hiding this 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
js-core/packages/shared/sdk-client/src/LDClientImpl.ts
Lines 613 to 642 in 2bf10a3
| 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'; |
Requirements
sdk-1653
sdk-1565
Describe the solution you've provided
LDDebugOverrideinterface to manage flag value overrides during development.safeRegisterDebugOverridePluginsfunction to register plugins with debug capabilities.FlagManagerto support debug overrides, including methods to set, remove, and clear overrides.LDClientImplto utilize debug overrides during client initialization.LDPlugininterface to include optionalregisterDebugmethod for plugins.This PR will enable
@launchdarkly/toolbarto use 4.xAdditional context
This PR is based off of feat: implementwaitForInitializationfor browser sdk 4.x #1028 (will need to do some rebase magic later)REVIEWERthat I would like some additional discussions before mergingNote
Introduces a debug flag override interface, wires plugin registration for overrides, and updates flag management and clients to support and emit override-driven changes.
LDDebugOverrideinterface and implement inDefaultFlagManager(setOverride,removeOverride,clearAllOverrides,getAllOverrides).get/getAllnow merge in-memory overrides with stored flags; override values take precedence."override"viaFlagUpdater.handleFlagChanges.FlagChangeTypewith"override"; addhandleFlagChangeshelper; refactorinit/upsertto use it.LDClientImpl: exposegetDebugOverrides()fromFlagManagerand continue inspection updates on changes.registerPlugins, callsafeRegisterDebugOverridePluginsto pass overrides to plugins.LDPluginBase.registerDebug(debugOverride)optional method.safeRegisterDebugOverridePluginsutility to safely register debug override plugins.LDPluginBase,LDDebugOverride, andsafeRegisterDebugOverridePluginsin public entrypoints.FlagManageroverride tests covering precedence, callbacks, clearing, and merging.Written by Cursor Bugbot for commit 2bf10a3. This will update automatically on new commits. Configure here.