-
Notifications
You must be signed in to change notification settings - Fork 41
feat: Migrate MultiProvider from js-sdk-contrib #1234
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?
feat: Migrate MultiProvider from js-sdk-contrib #1234
Conversation
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.
Summary of Changes
Hello @jonathannorris, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request integrates the MultiProvider feature directly into the OpenFeature SDK, enhancing its capability to manage and evaluate feature flags from multiple sources simultaneously. This migration provides a robust framework for scenarios like gradual migrations between flag providers, combining different data sources, or comparing flag values across systems. The implementation includes distinct server and web SDK components, complete with various evaluation strategies and comprehensive testing, streamlining multi-provider operations for developers.
Highlights
- Core Functionality Migration: The
MultiProvider
functionality has been migrated fromjs-sdk-contrib
directly into the main SDK, providing native support for managing multiple feature flag providers. - Platform-Specific Implementations: Dedicated
MultiProvider
implementations are now available for both server-side (MultiProvider
) and web-side (WebMultiProvider
) SDKs, optimized for their respective environments. - New Evaluation Strategies: Three distinct evaluation strategies are included:
FirstMatchStrategy
(default),FirstSuccessfulStrategy
, andComparisonStrategy
, offering flexible control over how flags are resolved across multiple providers. - Enhanced Test Coverage: Comprehensive test coverage has been added for both server and web implementations, ensuring reliability and stability of the new MultiProvider features.
- Updated Documentation: Detailed documentation has been updated for both server and web SDKs, providing clear usage examples and explanations of the new MultiProvider and its strategies.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
e388496
to
6b1d6d2
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.
Code Review
This pull request introduces the MultiProvider
functionality for both server and web SDKs, which is a fantastic addition. The implementation is robust and well-tested. I've identified a few areas for improvement, primarily in the documentation to ensure it accurately reflects the implementation, especially for the ComparisonStrategy
. Additionally, there are some minor opportunities for code refinement, such as removing redundant logging and a leftover TODO
comment. Overall, great work on this significant feature.
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.
Pull Request Overview
This PR migrates the MultiProvider functionality from the js-sdk-contrib repository into the main OpenFeature JS SDK, providing built-in support for multiple provider evaluation strategies. The implementation includes server and web variants with comprehensive testing and documentation.
Key Changes:
- Added
MultiProvider
for server SDK andWebMultiProvider
for web SDK with support for multiple evaluation strategies - Implemented three evaluation strategies: FirstMatch, FirstSuccessful, and Comparison
- Added comprehensive test coverage for both server and web implementations
- Updated documentation with usage examples and strategy explanations
Reviewed Changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 6 comments.
File | Description |
---|---|
packages/server/src/provider/multi-provider/ | Server implementation with async evaluation, hook handling, and tracking support |
packages/web/src/provider/multi-provider/ | Web implementation with synchronous evaluation and web-specific optimizations |
packages//test/multi-provider.spec.ts | Comprehensive test suites covering all strategies and edge cases |
packages/*/README.md | Updated documentation with usage examples and Multi-Provider sections |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/web/src/provider/multi-provider/strategies/first-successful-strategy.ts
Outdated
Show resolved
Hide resolved
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.
It looks like a good port of the multi-provider. As a follow up, it would be nice to remove some of the duplicate provider state logic.
packages/server/src/provider/multi-provider/strategies/base-evaluation-strategy.ts
Outdated
Show resolved
Hide resolved
packages/server/src/provider/multi-provider/strategies/base-evaluation-strategy.ts
Outdated
Show resolved
Hide resolved
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.
Left some feedback, I think we should take the chance to consolidate as much as possible to the core package.
private getStatusFromProviderStatuses() { | ||
const statuses = Object.values(this.providerStatuses); | ||
if (statuses.includes(ProviderStatus.FATAL)) { | ||
return ProviderStatus.FATAL; | ||
} else if (statuses.includes(ProviderStatus.NOT_READY)) { | ||
return ProviderStatus.NOT_READY; | ||
} else if (statuses.includes(ProviderStatus.ERROR)) { | ||
return ProviderStatus.ERROR; | ||
} else if (statuses.includes(ProviderStatus.STALE)) { | ||
return ProviderStatus.STALE; | ||
} | ||
return ProviderStatus.READY; | ||
} |
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.
Is there a reason RECONCILING
is not in here?
export class StatusTracker { | ||
private readonly providerStatuses: Record<string, ProviderStatus> = {}; |
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.
Currently this class is completely the same as in the server implementation besides the concrete types for provider and statuses.
It seems to me that the only reason this would change could be my question regarding RECONCILING
. Thinks like RegisteredProvider
should be easy to make compatible to both provider versions by using generics.
If my above assumption is correct I would tr to share the code here.
import { GeneralError, OpenFeatureError } from '@openfeature/core'; | ||
import type { RegisteredProvider } from './types'; | ||
|
||
export class ErrorWithCode extends OpenFeatureError { |
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.
This is also exactly the same as in the server, we can share this.
private registerProviders(constructorProviders: ProviderEntryInput[]) { | ||
const providersByName: Record<string, Provider[]> = {}; | ||
|
||
for (const constructorProvider of constructorProviders) { | ||
const providerName = constructorProvider.provider.metadata.name; | ||
const candidateName = constructorProvider.name ?? providerName; | ||
|
||
if (constructorProvider.name && providersByName[constructorProvider.name]) { | ||
throw new Error('Provider names must be unique'); | ||
} | ||
|
||
providersByName[candidateName] ??= []; | ||
providersByName[candidateName].push(constructorProvider.provider); | ||
} | ||
|
||
for (const name of Object.keys(providersByName)) { | ||
const useIndexedNames = providersByName[name].length > 1; | ||
for (let i = 0; i < providersByName[name].length; i++) { | ||
const indexedName = useIndexedNames ? `${name}-${i + 1}` : name; | ||
this.providerEntriesByName[indexedName] = { provider: providersByName[name][i], name: indexedName }; | ||
this.providerEntries.push(this.providerEntriesByName[indexedName]); | ||
this.statusTracker.wrapEventHandler(this.providerEntriesByName[indexedName]); | ||
} | ||
} |
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.
This and the track
method could live in a base MultiProvider
class in the shared folder.
return this.flagResolutionProxy(flagKey, 'object', defaultValue, context); | ||
} | ||
|
||
track(trackingEventName: string, context: EvaluationContext, trackingEventDetails: TrackingEventDetails): void { |
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.
As registerProviders
, this could be moved to a base MultiProvider
class.
/* eslint-disable @typescript-eslint/no-unused-vars */ | ||
import type { | ||
ErrorCode, | ||
EvaluationContext, | ||
FlagValue, | ||
FlagValueType, | ||
OpenFeatureError, | ||
ResolutionDetails, | ||
TrackingEventDetails, | ||
} from '@openfeature/core'; | ||
import type { Provider } from '../../provider'; | ||
import { ProviderStatus } from '../../provider'; | ||
import { ErrorWithCode } from '../errors'; | ||
|
||
export type StrategyEvaluationContext = { | ||
flagKey: string; | ||
flagType: FlagValueType; | ||
}; | ||
export type StrategyProviderContext = { | ||
provider: Provider; | ||
providerName: string; | ||
providerStatus: ProviderStatus; | ||
}; | ||
export type StrategyPerProviderContext = StrategyEvaluationContext & StrategyProviderContext; | ||
|
||
type ProviderResolutionResultBase = { | ||
provider: Provider; | ||
providerName: string; | ||
}; | ||
|
||
export type ProviderResolutionSuccessResult<T extends FlagValue> = ProviderResolutionResultBase & { | ||
details: ResolutionDetails<T>; | ||
}; | ||
|
||
export type ProviderResolutionErrorResult = ProviderResolutionResultBase & { | ||
thrownError: unknown; | ||
}; | ||
|
||
export type ProviderResolutionResult<T extends FlagValue> = | ||
| ProviderResolutionSuccessResult<T> | ||
| ProviderResolutionErrorResult; | ||
|
||
export type FinalResult<T extends FlagValue> = { | ||
details?: ResolutionDetails<T>; | ||
provider?: Provider; | ||
providerName?: string; | ||
errors?: { | ||
providerName: string; | ||
error: unknown; | ||
}[]; | ||
}; | ||
|
||
/** | ||
* Base strategy to inherit from. Not directly usable, as strategies must implement the "determineResult" method | ||
* Contains default implementations for `shouldEvaluateThisProvider` and `shouldEvaluateNextProvider` | ||
*/ | ||
export abstract class BaseEvaluationStrategy { | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
shouldEvaluateThisProvider(strategyContext: StrategyPerProviderContext, evalContext: EvaluationContext): boolean { | ||
if ( | ||
strategyContext.providerStatus === ProviderStatus.NOT_READY || | ||
strategyContext.providerStatus === ProviderStatus.FATAL | ||
) { | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
shouldEvaluateNextProvider<T extends FlagValue>( | ||
strategyContext: StrategyPerProviderContext, | ||
context: EvaluationContext, | ||
result: ProviderResolutionResult<T>, | ||
): boolean { | ||
return true; | ||
} | ||
|
||
shouldTrackWithThisProvider( | ||
strategyContext: StrategyProviderContext, | ||
context: EvaluationContext, | ||
trackingEventName: string, | ||
trackingEventDetails: TrackingEventDetails, | ||
): boolean { | ||
if ( | ||
strategyContext.providerStatus === ProviderStatus.NOT_READY || | ||
strategyContext.providerStatus === ProviderStatus.FATAL | ||
) { | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
abstract determineFinalResult<T extends FlagValue>( | ||
strategyContext: StrategyEvaluationContext, | ||
context: EvaluationContext, | ||
resolutions: ProviderResolutionResult<T>[], | ||
): FinalResult<T>; | ||
|
||
protected hasError(resolution: ProviderResolutionResult<FlagValue>): resolution is | ||
| ProviderResolutionErrorResult | ||
| (ProviderResolutionSuccessResult<FlagValue> & { | ||
details: ResolutionDetails<FlagValue> & { errorCode: ErrorCode }; | ||
}) { | ||
return 'thrownError' in resolution || !!resolution.details.errorCode; | ||
} | ||
|
||
protected hasErrorWithCode(resolution: ProviderResolutionResult<FlagValue>, code: ErrorCode): boolean { | ||
return 'thrownError' in resolution | ||
? (resolution.thrownError as OpenFeatureError)?.code === code | ||
: resolution.details.errorCode === code; | ||
} | ||
|
||
protected collectProviderErrors<T extends FlagValue>(resolutions: ProviderResolutionResult<T>[]): FinalResult<T> { | ||
const errors: FinalResult<FlagValue>['errors'] = []; | ||
for (const resolution of resolutions) { | ||
if ('thrownError' in resolution) { | ||
errors.push({ providerName: resolution.providerName, error: resolution.thrownError }); | ||
} else if (resolution.details.errorCode) { | ||
errors.push({ | ||
providerName: resolution.providerName, | ||
error: new ErrorWithCode(resolution.details.errorCode, resolution.details.errorMessage ?? 'unknown error'), | ||
}); | ||
} | ||
} | ||
return { errors }; | ||
} | ||
|
||
protected resolutionToFinalResult<T extends FlagValue>(resolution: ProviderResolutionSuccessResult<T>) { | ||
return { details: resolution.details, provider: resolution.provider, providerName: resolution.providerName }; | ||
} | ||
} |
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.
This is the same in client and server SDK.
The types are all the same besides Provider
, ProviderStatus
and ErrorWithCode
. We could just make them generic arguments of the class.
With this change, we can even use the strategies in both SDKs by moving them to the core package.
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.
This would create tighter coupling between both SDK's MultiProviders but would also enable us to share all strategies between SDKs, which I would prefer.
And I think for the 3 base strategies this is fine, they even share all the code today.
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.
yea if it's ok, I think I'm going to try and keep this as a mostly straight copy of the Multi-Provider from the contribs repo. But will work on refactoring these to be mostly common code.
cdc46b0
to
e82969f
Compare
Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
…event typings Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
…entation Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Jonathan Norris <[email protected]>
open-feature#1226) ## This PR - Added `useEffect` that runs on re-render to re-evaluate the flag value - Only updates state if the resolved value actually changed (using `isEqual` comparison) - Used lazy initialization for `useState` to avoid redundant initial evaluation - Added `useCallback` memoization for event handlers - Fixed `AbortController` scope issue ### Notes This resolves a subtle issue where the provider state may update without emitting a change event, leading to confusing results. The `useFlag` hook sets the initial evaluated value in a `useState`. Since this wasn't in a closure, this evaluation happened any time the component using the hook rerendered but the result was essentially ignored. Adding a logging hook shows that the current results but since this evaluation was made in an existing `useState`, the result had no effect. This resolves a subtle issue where the provider state may update without emitting a change event, leading to stale flag values being displayed. The `useFlag` hook was evaluating the flag on every re-render (as part of the `useState` initialization), but because `useState` only uses its initial value on the first render, these subsequent evaluations were being discarded. This meant that even though the hook was fetching the correct updated value from the provider on each re-render, it was throwing that value away and continuing to display the stale cached value. Adding a logging hook would show the correct evaluation happening (proving the provider had the updated value), but the UI would remain stuck with the old value because the `useState` was ignoring the re-evaluated result. The fix ensures that these re-evaluations on re-render actually update the component's state when the resolved value changes. The key insight is that the evaluation WAS happening on every re-render (due to how useState works), but React was discarding the result. Your fix makes those evaluations actually matter by checking if the value changed and updating state accordingly. Original thread: https://cloud-native.slack.com/archives/C06E4DE6S07/p1754508917397519 ### How to test I created a test that reproduced the issue, and it failed. I then implemented the changes and verified that the test passed. --------- Signed-off-by: Developer <[email protected]> Signed-off-by: Michael Beemer <[email protected]> Co-authored-by: Developer <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Lukas Reining <[email protected]> Co-authored-by: Todd Baert <[email protected]> Signed-off-by: Jonathan Norris <[email protected]>
Signed-off-by: Michael Beemer <[email protected]> Signed-off-by: Jonathan Norris <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [1.0.1](open-feature/js-sdk@react-sdk-v1.0.0...react-sdk-v1.0.1) (2025-08-18) ### 🐛 Bug Fixes * **react:** re-evaluate flags on re-render to detect silent provider … ([open-feature#1226](open-feature#1226)) ([3105595](open-feature@3105595)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <[email protected]> Signed-off-by: Jonathan Norris <[email protected]>
…ode. rename WebMultiProvider to MultiProvider Signed-off-by: Jonathan Norris <[email protected]>
e82969f
to
d254aad
Compare
Signed-off-by: Jonathan Norris <[email protected]>
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.
Pull Request Overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This PR migrates the
MultiProvider
functionality from the js-sdk-contrib repository into the main js-sdk, providing built-in support for multi-provider evaluation strategies. Addresses #1217What's Changed
MultiProvider
for server SDK with support for multiple evaluation strategiesWebMultiProvider
for web SDK with web-specific optimizationsTesting