-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Use SyncContext from SyncFlagsResponse instead of Metadata Request #1401
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: Use SyncContext from SyncFlagsResponse instead of Metadata Request #1401
Conversation
|
we do have e2e test for testing the sync context with gherkin, you can also enable those, to see if it is running properly :) https://github.com/open-feature/flagd-testbed/blob/1759a9491388e6c9714d5c048916120f77c8d892/gherkin/sync-payload.feature#L4 - those tests should normally work, if you add the missing provider configuration. |
libs/providers/flagd/src/lib/service/in-process/grpc/grpc-fetch.ts
Outdated
Show resolved
Hide resolved
|
You're on the right track, but I think you need to add the transformation function I mentioned here. |
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
8888f90 to
ed148f2
Compare
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
toddbaert
left a 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.
Looks great! You only have one small issue, but it's one that will break things so I'm blocking for now: https://github.com/open-feature/js-sdk-contrib/pull/1401/files#r2546237260
|
/gemini review |
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 integrates the SyncContext from the SyncFlagsResponse into the evaluation context, which is a valuable feature. The implementation introduces a new SyncMetadataHook and a contextEnricher configuration option. The overall approach is sound, but I've identified a critical issue with function binding that would lead to runtime errors, a couple of high-severity issues related to type safety and null handling, and a medium-severity suggestion to improve code clarity. Addressing these points will enhance the robustness of this new feature.
Signed-off-by: marcozabel <[email protected]>
|
we do have an e2e test within the test-bed for this. it seems like the submodule for the testbed in libs/shared/flagd-core is still pointing to an older version which does not contain those tests. can you please update the submodule to at least version https://github.com/open-feature/flagd-testbed/releases/tag/v2.11.0 - where those tests are added to verify this changes are correct. (a newer version would be even better but not needed) |
Signed-off-by: marcozabel <[email protected]>
Signed-off-by: marcozabel <[email protected]>
This PR
Related Issues
Fixes #1350