-
Notifications
You must be signed in to change notification settings - Fork 31
feat: allow clients to evaluate bootstrapped flags when not ready #1036
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { | |
| Platform, | ||
| } from '@launchdarkly/js-client-sdk-common'; | ||
|
|
||
| import { readFlagsFromBootstrap } from './bootstrap'; | ||
| import { getHref } from './BrowserApi'; | ||
| import BrowserDataManager from './BrowserDataManager'; | ||
| import { BrowserIdentifyOptions as LDIdentifyOptions } from './BrowserIdentifyOptions'; | ||
|
|
@@ -44,6 +45,10 @@ class BrowserClientImpl extends LDClientImpl { | |
| private _initResolve?: (result: LDWaitForInitializationResult) => void; | ||
| private _initializeResult?: LDWaitForInitializationResult; | ||
|
|
||
| // NOTE: keeps track of when we tried an initial identification. We should consolidate this | ||
| // with the waitForInitialization logic in the future. | ||
| private _identifyAttempted: boolean = false; | ||
|
|
||
| constructor( | ||
| clientSideId: string, | ||
| autoEnvAttributes: AutoEnvAttributes, | ||
|
|
@@ -222,6 +227,20 @@ class BrowserClientImpl extends LDClientImpl { | |
| if (identifyOptions?.sheddable === undefined) { | ||
| identifyOptionsWithUpdatedDefaults.sheddable = true; | ||
| } | ||
|
|
||
| if (!this._identifyAttempted && identifyOptionsWithUpdatedDefaults.bootstrap) { | ||
| this._identifyAttempted = true; | ||
| const bootstrapData = readFlagsFromBootstrap( | ||
| this.logger, | ||
| identifyOptionsWithUpdatedDefaults.bootstrap, | ||
| ); | ||
| try { | ||
| this.presetFlags(bootstrapData); | ||
| } catch { | ||
| this.logger.error('Failed to bootstrap data'); | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Bootstrap preset flag logic executes on wrong identify callThe |
||
|
|
||
| const res = await super.identifyResult(context, identifyOptionsWithUpdatedDefaults); | ||
| if (res.status === 'completed') { | ||
| this._initializeResult = { status: 'complete' }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,10 @@ import { LDIdentifyOptions } from './api/LDIdentifyOptions'; | |
| import { createAsyncTaskQueue } from './async/AsyncTaskQueue'; | ||
| import { Configuration, ConfigurationImpl, LDClientInternalOptions } from './configuration'; | ||
| import { addAutoEnv } from './context/addAutoEnv'; | ||
| import { | ||
| ActiveContextTracker, | ||
| createActiveContextTracker, | ||
| } from './context/createActiveContextTracker'; | ||
| import { ensureKey } from './context/ensureKey'; | ||
| import { DataManager, DataManagerFactory } from './DataManager'; | ||
| import createDiagnosticsManager from './diagnostics/createDiagnosticsManager'; | ||
|
|
@@ -43,6 +47,7 @@ import createEventProcessor from './events/createEventProcessor'; | |
| import EventFactory from './events/EventFactory'; | ||
| import DefaultFlagManager, { FlagManager } from './flag-manager/FlagManager'; | ||
| import { FlagChangeType } from './flag-manager/FlagUpdater'; | ||
| import { ItemDescriptor } from './flag-manager/ItemDescriptor'; | ||
| import HookRunner from './HookRunner'; | ||
| import { getInspectorHook } from './inspection/getInspectorHook'; | ||
| import InspectorManager from './inspection/InspectorManager'; | ||
|
|
@@ -55,12 +60,12 @@ const DEFAULT_IDENTIFY_TIMEOUT_SECONDS = 5; | |
|
|
||
| export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | ||
| private readonly _config: Configuration; | ||
| private _uncheckedContext?: LDContext; | ||
| private _checkedContext?: Context; | ||
| private readonly _diagnosticsManager?: internal.DiagnosticsManager; | ||
| private _eventProcessor?: internal.EventProcessor; | ||
| readonly logger: LDLogger; | ||
|
|
||
| private _activeContextTracker: ActiveContextTracker = createActiveContextTracker(); | ||
|
|
||
| private readonly _highTimeoutThreshold: number = 15; | ||
|
|
||
| private _eventFactoryDefault = new EventFactory(false); | ||
|
|
@@ -200,27 +205,22 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| // code. We are returned the unchecked context so that if a consumer identifies with an invalid context | ||
| // and then calls getContext, they get back the same context they provided, without any assertion about | ||
| // validity. | ||
| return this._uncheckedContext ? clone<LDContext>(this._uncheckedContext) : undefined; | ||
| return this._activeContextTracker.hasContext() | ||
| ? clone<LDContext>(this._activeContextTracker.getPristineContext()) | ||
| : undefined; | ||
| } | ||
|
|
||
| protected getInternalContext(): Context | undefined { | ||
| return this._checkedContext; | ||
| return this._activeContextTracker.getContext(); | ||
| } | ||
|
|
||
| private _createIdentifyPromise(): { | ||
| identifyPromise: Promise<void>; | ||
| identifyResolve: () => void; | ||
| identifyReject: (err: Error) => void; | ||
| } { | ||
| let res: any; | ||
| let rej: any; | ||
|
|
||
| const basePromise = new Promise<void>((resolve, reject) => { | ||
| res = resolve; | ||
| rej = reject; | ||
| }); | ||
|
|
||
| return { identifyPromise: basePromise, identifyResolve: res, identifyReject: rej }; | ||
| /** | ||
| * Preset flags are used to set the flags before the client is initialized. This is useful for | ||
| * when client has precached flags that are ready to evaluate without full initialization. | ||
| * @param newFlags - The flags to preset. | ||
| */ | ||
| protected presetFlags(newFlags: { [key: string]: ItemDescriptor }) { | ||
| this._flagManager.presetFlags(newFlags); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -307,15 +307,14 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| this.emitter.emit('error', context, error); | ||
| return Promise.reject(error); | ||
| } | ||
| this._uncheckedContext = context; | ||
| this._checkedContext = checkedContext; | ||
| this._activeContextTracker.set(context, checkedContext); | ||
|
|
||
| this._eventProcessor?.sendEvent( | ||
| this._eventFactoryDefault.identifyEvent(this._checkedContext), | ||
| this._eventFactoryDefault.identifyEvent(checkedContext), | ||
| ); | ||
| const { identifyPromise, identifyResolve, identifyReject } = | ||
| this._createIdentifyPromise(); | ||
| this.logger.debug(`Identifying ${JSON.stringify(this._checkedContext)}`); | ||
| this._activeContextTracker.newIdentificationPromise(); | ||
| this.logger.debug(`Identifying ${JSON.stringify(checkedContext)}`); | ||
|
|
||
| await this.dataManager.identify( | ||
| identifyResolve, | ||
|
|
@@ -370,7 +369,7 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| } | ||
|
|
||
| track(key: string, data?: any, metricValue?: number): void { | ||
| if (!this._checkedContext || !this._checkedContext.valid) { | ||
| if (!this._activeContextTracker.hasValidContext()) { | ||
| this.logger.warn(ClientMessages.MissingContextKeyNoEvent); | ||
| return; | ||
| } | ||
|
|
@@ -382,14 +381,19 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
|
|
||
| this._eventProcessor?.sendEvent( | ||
| this._config.trackEventModifier( | ||
| this._eventFactoryDefault.customEvent(key, this._checkedContext!, data, metricValue), | ||
| this._eventFactoryDefault.customEvent( | ||
| key, | ||
| this._activeContextTracker.getContext()!, | ||
| data, | ||
| metricValue, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| this._hookRunner.afterTrack({ | ||
| key, | ||
| // The context is pre-checked above, so we know it can be unwrapped. | ||
| context: this._uncheckedContext!, | ||
| context: this._activeContextTracker.getPristineContext()!, | ||
| data, | ||
| metricValue, | ||
| }); | ||
|
|
@@ -401,23 +405,34 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| eventFactory: EventFactory, | ||
| typeChecker?: (value: any) => [boolean, string], | ||
| ): LDEvaluationDetail { | ||
| if (!this._uncheckedContext) { | ||
| this.logger.debug(ClientMessages.MissingContextKeyNoEvent); | ||
| return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue); | ||
| // We are letting evaulations happen without a context. The main case for this | ||
| // is when cached data is loaded, but the client is not fully initialized. In this | ||
| // case, we will write out a warning for each evaluation attempt. | ||
|
|
||
| // NOTE: we will be changing this behavior soon once we have a tracker on the | ||
| // client initialization state. | ||
| const hasContext = this._activeContextTracker.hasContext(); | ||
| if (!hasContext) { | ||
| this.logger?.warn( | ||
| 'Flag evaluation called before client is fully initialized, data from this evaulation could be stale.', | ||
| ); | ||
| } | ||
|
|
||
| const evalContext = Context.fromLDContext(this._uncheckedContext); | ||
| const evalContext = this._activeContextTracker.getContext()!; | ||
| const foundItem = this._flagManager.get(flagKey); | ||
|
|
||
| if (foundItem === undefined || foundItem.flag.deleted) { | ||
| const defVal = defaultValue ?? null; | ||
| const error = new LDClientError( | ||
| `Unknown feature flag "${flagKey}"; returning default value ${defVal}.`, | ||
| ); | ||
| this.emitter.emit('error', this._uncheckedContext, error); | ||
| this._eventProcessor?.sendEvent( | ||
| this._eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext), | ||
| ); | ||
|
|
||
| this.emitter.emit('error', this._activeContextTracker.getPristineContext(), error); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Error events emitted with undefined context during pre-initialization evaluationWhen evaluating flags before identification completes (using preset bootstrap data), error events for "flag not found" or "wrong type" are emitted via Additional Locations (1) |
||
| if (hasContext) { | ||
| this._eventProcessor?.sendEvent( | ||
| this._eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext), | ||
| ); | ||
| } | ||
| return createErrorEvaluationDetail(ErrorKinds.FlagNotFound, defaultValue); | ||
| } | ||
|
|
||
|
|
@@ -426,20 +441,22 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| if (typeChecker) { | ||
| const [matched, type] = typeChecker(value); | ||
| if (!matched) { | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| defaultValue, // track default value on type errors | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| if (hasContext) { | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| defaultValue, // track default value on type errors | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| } | ||
| const error = new LDClientError( | ||
| `Wrong type "${type}" for feature flag "${flagKey}"; returning default value`, | ||
| ); | ||
| this.emitter.emit('error', this._uncheckedContext, error); | ||
| this.emitter.emit('error', this._activeContextTracker.getPristineContext(), error); | ||
| return createErrorEvaluationDetail(ErrorKinds.WrongType, defaultValue); | ||
| } | ||
| } | ||
|
|
@@ -453,31 +470,36 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| prerequisites?.forEach((prereqKey) => { | ||
| this._variationInternal(prereqKey, undefined, this._eventFactoryDefault); | ||
| }); | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| value, | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| if (hasContext) { | ||
| this._eventProcessor?.sendEvent( | ||
| eventFactory.evalEventClient( | ||
| flagKey, | ||
| value, | ||
| defaultValue, | ||
| foundItem.flag, | ||
| evalContext, | ||
| reason, | ||
| ), | ||
| ); | ||
| } | ||
| return successDetail; | ||
| } | ||
|
|
||
| variation(flagKey: string, defaultValue?: LDFlagValue): LDFlagValue { | ||
| const { value } = this._hookRunner.withEvaluation( | ||
| flagKey, | ||
| this._uncheckedContext, | ||
| this._activeContextTracker.getPristineContext(), | ||
| defaultValue, | ||
| () => this._variationInternal(flagKey, defaultValue, this._eventFactoryDefault), | ||
| ); | ||
| return value; | ||
| } | ||
| variationDetail(flagKey: string, defaultValue?: LDFlagValue): LDEvaluationDetail { | ||
| return this._hookRunner.withEvaluation(flagKey, this._uncheckedContext, defaultValue, () => | ||
| this._variationInternal(flagKey, defaultValue, this._eventFactoryWithReasons), | ||
| return this._hookRunner.withEvaluation( | ||
| flagKey, | ||
| this._activeContextTracker.getPristineContext(), | ||
| defaultValue, | ||
| () => this._variationInternal(flagKey, defaultValue, this._eventFactoryWithReasons), | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -487,8 +509,11 @@ export default class LDClientImpl implements LDClient, LDClientIdentifyResult { | |
| eventFactory: EventFactory, | ||
| typeChecker: (value: unknown) => [boolean, string], | ||
| ): LDEvaluationDetailTyped<T> { | ||
| return this._hookRunner.withEvaluation(key, this._uncheckedContext, defaultValue, () => | ||
| this._variationInternal(key, defaultValue, eventFactory, typeChecker), | ||
| return this._hookRunner.withEvaluation( | ||
| key, | ||
| this._activeContextTracker.getPristineContext(), | ||
| defaultValue, | ||
| () => this._variationInternal(key, defaultValue, eventFactory, typeChecker), | ||
| ); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.