-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: support metadata in errors in OFREP #1203
Conversation
toddbaert
commented
Feb 5, 2025
•
edited
Loading
edited
- uses flag-set metadata to return metadata in OFREP errors (best effort)
- removes some duplicated enums from OFREP
- sets @openfeature/ofrep-core package to v1.0.0
66fe4f0
to
8a2a9c3
Compare
8a2a9c3
to
3ed58d6
Compare
3ed58d6
to
e033170
Compare
3ef0d71
to
b520c12
Compare
a356f3a
to
04b7488
Compare
b9ac7e1
to
3558d0d
Compare
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
3558d0d
to
b0de3b2
Compare
Signed-off-by: Todd Baert <[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.
Looks good to me! Left nits only.
@@ -84,25 +85,32 @@ export class OFREPProvider implements Provider { | |||
|
|||
try { | |||
const result = await this.ofrepApi.postEvaluateFlag(flagKey, { context }); | |||
return this.toResolutionDetails(result, defaultValue); | |||
return this.responseToResolutionDetails(result, defaultValue); |
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.
Why the change? I find the name a bit confusing.
I would argue that toResolutionDetails
matches good.
export type EvaluationFlagValue = FlagValue; | ||
|
||
export interface EvaluationSuccessResponse { | ||
export interface MetadataResponse { |
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.
Nit: I would not expect this to have the Response
suffix, rather something like EvaluationResponseMetadata
, but it might also be a foreign language problem for me :D