-
Notifications
You must be signed in to change notification settings - Fork 54
refactor: move Multi-Provider shared code to core package #1324
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?
Conversation
Summary of ChangesHello @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 significantly refactors the Multi-Provider implementation by extracting shared logic and types from the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
…types Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
c24d7e6 to
52d8268
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 is a well-executed refactoring that consolidates the Multi-Provider logic from the server and web packages into a shared @openfeature/core package. Using generics to support both SDKs from a single implementation is a great approach that significantly reduces code duplication and improves maintainability. The changes are logical and the test updates are thorough. My review includes a few suggestions to clean up some leftover files and remove redundant type assertions, which will help finalize this excellent refactoring.
- Remove redundant 'provider as Provider' casts where type is already known - Add proper type definitions for StatusTracker enums to eliminate 'as TProviderEvents' casts - Use variable narrowing for optional Reconciling event/status Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
These files are now obsolete as the types are imported from the shared @openfeature/core package. Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
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 consolidates duplicate Multi-Provider code from the server and web packages into the @openfeature/core shared package, reducing code duplication by approximately 275 lines. The refactoring uses generics (TProviderStatus, TProvider) to support both server and web SDKs from a single implementation.
Key changes:
- Moved evaluation strategies, status tracking, and error utilities to the shared package with generic type parameters
- Updated
AllProviderStatusenum to explicitly include all status values (includingRECONCILINGfor web) - Modified strategy constructors to require
ProviderStatusenum parameter for platform-specific status handling
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/shared/src/provider/provider.ts |
Updated AllProviderStatus enum to be explicit rather than an alias, including all status values |
packages/shared/src/provider/multi-provider/types.ts |
Added generic type definitions for ProviderEntryInput and RegisteredProvider |
packages/shared/src/provider/multi-provider/strategies/base-evaluation-strategy.ts |
Created generic base strategy class with type parameters for provider status and provider type |
packages/shared/src/provider/multi-provider/strategies/first-match-strategy.ts |
Added generic implementation of first-match strategy |
packages/shared/src/provider/multi-provider/strategies/first-successful-strategy.ts |
Added generic implementation of first-successful strategy |
packages/shared/src/provider/multi-provider/strategies/comparison-strategy.ts |
Added generic implementation of comparison strategy with updated constructor signature |
packages/shared/src/provider/multi-provider/status-tracker.ts |
Created generic status tracker with type parameters for events, status, and provider |
packages/shared/src/provider/multi-provider/errors.ts |
Moved error utilities with improved null-safety in constructAggregateError |
packages/shared/src/provider/index.ts |
Added exports for multi-provider components |
packages/web/src/provider/multi-provider/multi-provider-web.ts |
Updated to use shared types and strategies with proper generic parameters |
packages/web/src/provider/multi-provider/index.ts |
Removed local exports now available from shared package |
packages/web/test/multi-provider-web.spec.ts |
Updated strategy instantiations to pass ProviderStatus enum parameter |
packages/server/src/provider/multi-provider/multi-provider.ts |
Updated to use shared types and strategies with proper generic parameters |
packages/server/src/provider/multi-provider/index.ts |
Removed local exports now available from shared package |
packages/server/test/multi-provider.spec.ts |
Updated strategy instantiations to pass ProviderStatus enum parameter |
| (deleted files) | Removed duplicate implementations of strategies, status tracker, errors, and types from both web and server packages |
Comments suppressed due to low confidence (4)
packages/shared/src/provider/multi-provider/errors.ts:28
- The
.flat()call on line 28 is unnecessary. Themapfunction returns an array of objects, each already at the top level, so there are no nested arrays to flatten. This is likely a redundant operation left from previous refactoring.
packages/shared/src/provider/multi-provider/strategies/first-match-strategy.ts:6 - Documentation comments describing the strategy's behavior have been removed during the refactoring. The
FirstMatchStrategyclass should include a JSDoc comment explaining that it returns the first result that did not indicate "flag not found", and throws an error if any provider in the course of evaluation returns or throws an error.
packages/shared/src/provider/multi-provider/strategies/first-successful-strategy.ts:8 - Documentation comments describing the strategy's behavior have been removed during the refactoring. The
FirstSuccessfulStrategyclass should include a JSDoc comment explaining that it returns the first result that did NOT result in an error, ignores errors as long as there is a successful result, and throws all errors if there is no successful result.
packages/shared/src/provider/multi-provider/strategies/comparison-strategy.ts:11 - Documentation comments describing the strategy's behavior have been removed during the refactoring. The
ComparisonStrategyclass should include a JSDoc comment explaining that it evaluates all providers in parallel and compares the results. If values agree, it returns the value; if they disagree, it returns the value from the configured "fallback provider" and executes the "onMismatch" callback if defined.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/shared/src/provider/multi-provider/strategies/base-evaluation-strategy.ts
Show resolved
Hide resolved
- Add comprehensive tests for StatusTracker event handling and status priority - Add tests for constructAggregateError including empty array edge case - Add tests for throwAggregateErrorFromPromiseResults - Fix AggregateError prototype chain for proper instanceof checks Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
Keep AllProviderStatus as an alias of ClientProviderStatus instead of creating a new enum, to maintain backward compatibility. Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
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 27 out of 28 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ic API - Create FirstMatchStrategy, FirstSuccessfulStrategy, ComparisonStrategy wrapper classes in server and web SDKs with ProviderStatus pre-bound - Rename base strategies to BaseFirstMatchStrategy, etc. in @openfeature/core - Update READMEs with simplified custom strategy documentation - Users can now use strategies without passing ProviderStatus: new FirstMatchStrategy() instead of new FirstMatchStrategy(ProviderStatus) Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
849f66a to
97eb2ea
Compare
Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
|
I think I found just one small issue here. You added some generic args to some of the components for building custom strats, to support the divergence in types between web/server (we do this a lot). However, I think we can make things easier for consumers by re-exporting these from server/web with the relevant generics already passed, since we know what they will have to be in those modules. I opened this PR to this branch. I hope that makes sense: #1327 |
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.
I'm approving, but please consider this.
See: #1324 (comment) --------- Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Summary
serverandwebpackages into@openfeature/core(shared package)TProviderStatus,TProvider) internally to support both server and web SDKs from a single implementationChanges
Moved to shared package (
@openfeature/core):BaseEvaluationStrategyand base strategy classesStatusTrackerfor provider status managementAggregateError,constructAggregateError)ProviderEntryInput,RegisteredProvider)SDK-specific strategy exports:
FirstMatchStrategy,FirstSuccessfulStrategy, andComparisonStrategyclassesProviderStatuspre-bound, maintaining backward compatibilitynew FirstMatchStrategy()without any changesNew tests:
StatusTracker(event handling, status priority)constructAggregateError(including edge cases)Documentation:
BaseEvaluationStrategyclass structure documentation