-
Notifications
You must be signed in to change notification settings - Fork 25
feat: implement context values #203
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
Segments | ||
} from './evaluationContext.types.ts'; | ||
|
||
export type EnvironmentKey = EnvironmentContext['key']; |
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.
Added this manually to avoid relying on auto-generated redundant name keys:
export type Key1 = string;
/**
* Key used for % split segmentation.
*/
export type Key2 = string;
/**
flagsmith-engine/index.ts
Outdated
const { segments, segmentOverrides } = evaluateSegments(context); | ||
const flags = evaluateFeatures(context, segmentOverrides); | ||
|
||
// Not sure if we need this - Keeping till confirmed hidedisabledflags is remote evaluation only |
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.
Question here about hidedisabled-flags. can remove
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.
Yup — I believe this is cleared up now; we don't use the setting for local evaluation mode.
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.
In general, the PR looks solid to me — just a handful of comments to address.
/** | ||
* A context object containing the necessary information to evaluate Flagsmith feature flags. | ||
*/ | ||
export interface EvaluationContext { |
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.
Any way to avoid the type duplication here?
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.
Yep, I created a joint model type at the root of evaluation folder. The idea being that:
- All types used in the code must come from this
evaluation/models.ts
- If a type is needed from one generated file, it must be imported and re-exported from this entrypoint
flagsmith-engine/index.ts
Outdated
const { segments, segmentOverrides } = evaluateSegments(context); | ||
const flags = evaluateFeatures(context, segmentOverrides); | ||
|
||
// Not sure if we need this - Keeping till confirmed hidedisabledflags is remote evaluation only |
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.
Yup — I believe this is cleared up now; we don't use the setting for local evaluation mode.
Closes #198
In this PR, we:
EvaluationResult getEvaluationResult(EvaluationContext context)
and remove all of the old engine APIs.evaluationContext/mappers
for engine-API data interopgenerate-engine-types
to generate fromflagsmith/sdk/evaluation-*-schema
-> in pre-commitCouple of questions concerning:
hide_disabled_flags
deprecation => ready to remove the commented codegetEnvironment
that also includesgetEnvironmentFromApi
. I'm not seeing a huge value to migrate it but maybe I am missing something ? this would getEnvironmentFlagsFromDocument/getIdentityFlagsFromDocument -> getEnvironment -> this.offlineHandler.getEnvironment -> getEvaluationContext (using environment)Happy to change if misunderstood