-
Notifications
You must be signed in to change notification settings - Fork 66
feat(core): pass bound domain to provider initialize and enforce domain-scoped binding #1433
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 all commits
e8e9c75
98752ff
cb04d90
0b58f77
50bdb4d
da94f44
0be5c32
b964fe0
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 |
|---|---|---|
|
|
@@ -231,6 +231,12 @@ export abstract class OpenFeatureCommonAPI< | |
|
|
||
| // ignore no-ops | ||
| if (oldProvider === provider) { | ||
| // domain-scoped providers may already be the default; an explicit domain bind is not a no-op | ||
| if (provider.domainScoped && domain && this._domainScopedProviders.get(domain)?.provider !== provider) { | ||
| throw new GeneralError( | ||
| `Cannot bind domain-scoped provider '${provider.metadata.name}' to more than one domain.`, | ||
| ); | ||
| } | ||
| this._logger.debug('Provider is already set, ignoring setProvider call'); | ||
| return; | ||
| } | ||
|
|
@@ -241,6 +247,10 @@ export abstract class OpenFeatureCommonAPI< | |
| throw new GeneralError(`Provider '${provider.metadata.name}' is intended for use on the ${provider.runsOn}.`); | ||
| } | ||
|
|
||
| if (provider.domainScoped && this.allProviders.includes(provider)) { | ||
| throw new GeneralError(`Cannot bind domain-scoped provider '${provider.metadata.name}' to more than one domain.`); | ||
| } | ||
|
|
||
|
Comment on lines
234
to
+253
Member
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. Nit: I think the "domainScoped" check could be consolidated into a single upfront check that inspects both Totally optional, only do it if you agree. |
||
| const emitters = this.getAssociatedEventEmitters(domain); | ||
|
|
||
| let initializationPromise: Promise<void> | void = undefined; | ||
|
|
@@ -252,8 +262,9 @@ export abstract class OpenFeatureCommonAPI< | |
|
|
||
| // initialize the provider if it implements "initialize" and it's not already registered | ||
| if (typeof provider.initialize === 'function' && !this.allProviders.includes(provider)) { | ||
| const initContext = domain ? (this._domainScopedContext.get(domain) ?? this._context) : this._context; | ||
| initializationPromise = provider | ||
| .initialize?.(domain ? (this._domainScopedContext.get(domain) ?? this._context) : this._context) | ||
| .initialize?.(initContext, domain) | ||
| ?.then(() => { | ||
|
jonathannorris marked this conversation as resolved.
|
||
| wrappedProvider.status = this._statusEnumType.READY; | ||
| // fetch the most recent event emitters, some may have been added during init | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,12 @@ export interface CommonProvider<S extends ClientProviderStatus | ServerProviderS | |
| */ | ||
| readonly runsOn?: Paradigm; | ||
|
|
||
| /** | ||
| * When true, the provider maintains state specific to a single domain (such as a | ||
| * persistent cache) and the SDK will bind it to at most one domain. | ||
| */ | ||
| readonly domainScoped?: boolean; | ||
|
|
||
| // TODO: in the future we could make this a never to force provider to remove it. | ||
| /** | ||
| * @deprecated the SDK now maintains the provider's state; there's no need for providers to implement this field. | ||
|
|
@@ -123,9 +129,10 @@ export interface CommonProvider<S extends ClientProviderStatus | ServerProviderS | |
| * When the returned promise resolves, the SDK fires the ProviderEvents.Ready event. | ||
| * If the returned promise rejects, the SDK fires the ProviderEvents.Error event. | ||
| * Use this function to perform any context-dependent setup within the provider. | ||
| * @param context | ||
| * @param context the global evaluation context | ||
| * @param domain the bound domain, if any | ||
| */ | ||
| initialize?(context?: EvaluationContext): Promise<void>; | ||
| initialize?(context?: EvaluationContext, domain?: string): Promise<void>; | ||
|
jonathannorris marked this conversation as resolved.
Member
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. I wonder if we should make this an initialization config object so we can extend it in the future without breaking anything.
Member
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. I don't think it matters as much here due to overloading, but it's possible and maybe a good idea. We could have an |
||
|
|
||
| /** | ||
| * Track a user action or application state, usually representing a business objective or outcome. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,73 @@ | ||||||||||||||||||||||||
| import type { EvaluationContext, Paradigm } from '../src'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| type LegacyInitializeProviderOptions = { | ||||||||||||||||||||||||
| runsOn: Paradigm; | ||||||||||||||||||||||||
| name?: string; | ||||||||||||||||||||||||
| /** When true, resolution stubs return promises (server SDK). Default false (web SDK). */ | ||||||||||||||||||||||||
| asyncResolvers?: boolean; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| type LegacyInitializeProviderExtras = { | ||||||||||||||||||||||||
| events: unknown; | ||||||||||||||||||||||||
| hooks?: unknown[]; | ||||||||||||||||||||||||
| track?: jest.Mock; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| type LegacyInitializeProviderResolvers = { | ||||||||||||||||||||||||
| resolveBooleanEvaluation: jest.Mock; | ||||||||||||||||||||||||
| resolveStringEvaluation: jest.Mock; | ||||||||||||||||||||||||
| resolveNumberEvaluation: jest.Mock; | ||||||||||||||||||||||||
| resolveObjectEvaluation: jest.Mock; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| export type LegacyInitializeProvider = LegacyInitializeProviderResolvers & { | ||||||||||||||||||||||||
| metadata: { name: string }; | ||||||||||||||||||||||||
| runsOn: Paradigm; | ||||||||||||||||||||||||
| lastContext?: EvaluationContext; | ||||||||||||||||||||||||
| initializeCalls: number; | ||||||||||||||||||||||||
| initialize: (context?: EvaluationContext) => Promise<void>; | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function createResolverStubs(asyncResolvers: boolean): LegacyInitializeProviderResolvers { | ||||||||||||||||||||||||
| const mockValue = asyncResolvers | ||||||||||||||||||||||||
| ? <T>(value: T) => jest.fn().mockResolvedValue(value) | ||||||||||||||||||||||||
| : <T>(value: T) => jest.fn().mockReturnValue(value); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||
| resolveBooleanEvaluation: mockValue({ value: false }), | ||||||||||||||||||||||||
| resolveStringEvaluation: mockValue({ value: '' }), | ||||||||||||||||||||||||
| resolveNumberEvaluation: mockValue({ value: 0 }), | ||||||||||||||||||||||||
| resolveObjectEvaluation: mockValue({ value: {} }), | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
|
Check warning on line 44 in packages/shared/test/legacy-initialize-provider.ts
|
||||||||||||||||||||||||
| * Provider with a single-argument initialize that ignores any extra arguments passed by the SDK. | ||||||||||||||||||||||||
| * Pass optional extras when MultiProvider needs events, hooks, or track stubs on the child. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
|
Comment on lines
+44
to
+47
Member
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.
Suggested change
Fixes a lint. |
||||||||||||||||||||||||
| export function legacyInitializeProvider( | ||||||||||||||||||||||||
| options: LegacyInitializeProviderOptions, | ||||||||||||||||||||||||
| extras?: LegacyInitializeProviderExtras, | ||||||||||||||||||||||||
| ): LegacyInitializeProvider { | ||||||||||||||||||||||||
| const provider: LegacyInitializeProvider = { | ||||||||||||||||||||||||
| metadata: { name: options.name ?? 'legacy-init' }, | ||||||||||||||||||||||||
| runsOn: options.runsOn, | ||||||||||||||||||||||||
| lastContext: undefined, | ||||||||||||||||||||||||
| initializeCalls: 0, | ||||||||||||||||||||||||
| async initialize(context?: EvaluationContext): Promise<void> { | ||||||||||||||||||||||||
| this.lastContext = context; | ||||||||||||||||||||||||
| this.initializeCalls++; | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| ...createResolverStubs(options.asyncResolvers ?? false), | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (extras) { | ||||||||||||||||||||||||
| Object.assign(provider, { | ||||||||||||||||||||||||
| events: extras.events, | ||||||||||||||||||||||||
| hooks: extras.hooks ?? [], | ||||||||||||||||||||||||
| track: extras.track ?? jest.fn(), | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return provider; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.