Skip to content
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

fix: flag not found when using dependency flag in targeting rule #95

Merged
merged 7 commits into from
Mar 10, 2025

Conversation

duyhungtnn
Copy link
Collaborator

@duyhungtnn duyhungtnn commented Mar 4, 2025

Part of bucketeer-io/bucketeer#1552

Refers to https://github.com/bucketeer-io/go-server-sdk/pull/157/files

This pull request includes several changes to improve the evaluation process and testing in the project. The most important changes include updating dependencies, refactoring test cases, and enhancing the LocalEvaluator class.

Dependency updates:

  • Updated the version of @bucketeer/evaluation from 0.0.1 to 0.0.3 in the package.json file.

Test improvements:

  • Refactored imports in src/__tests__/client_assert_get_evaluation_rq.ts to directly import BKTClientImpl from client.

  • Added new test cases for protoReasonToReason function in src/__tests__/evaluator/protoReasonToReason.ts.

  • Modified multiple test cases in src/__tests__/evaluator/evaluator.ts to use try-catch blocks instead of .catch for error handling. [1] [2] [3] [4] [5]
    Their tests will fail if the expected error is not throw

  • Added new test cases for getTargetFeatures, findEvaluation, and getTargetFeatures methods in src/__tests__/evaluator/evaluator.ts.

Enhancements to LocalEvaluator class:

  • Modified the findEvaluation method to be public.
  • Refactored the getTargetFeatures method to handle prerequisite features more efficiently.
  • Added a new private method getPrerequisiteFeaturesFromCache to replace getPrerequisiteFeatures.
  • Exported protoReasonToReason function from src/evaluator/local.ts.

@duyhungtnn duyhungtnn force-pushed the fix-flag-not-found branch from 55e26df to f9fdd1b Compare March 7, 2025 09:21
@duyhungtnn duyhungtnn marked this pull request as ready for review March 7, 2025 09:22
@duyhungtnn duyhungtnn requested a review from cre8ivejp as a code owner March 7, 2025 09:22
@duyhungtnn duyhungtnn force-pushed the fix-flag-not-found branch from f9fdd1b to d369990 Compare March 7, 2025 09:27
@duyhungtnn duyhungtnn force-pushed the fix-flag-not-found branch from 3925425 to 0d726a7 Compare March 7, 2025 14:06
@duyhungtnn
Copy link
Collaborator Author

@cre8ivejp please help me to take a look.

@duyhungtnn duyhungtnn self-assigned this Mar 7, 2025
}
const prerequisiteFeatures = await this.getPrerequisiteFeatures(feature);

const prerequisiteFeatures = await this.getPrerequisiteFeaturesFromCache(preFlagIDs);
return targetFeatures.concat(prerequisiteFeatures);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming, this concat will add the list after the this flag, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, its the same with this go code

https://github.com/bucketeer-io/go-server-sdk/blob/master/pkg/bucketeer/evaluator/evaluator.go#L113-L115

But I updated the code for more easy readable using new typescript syntax [...]

  async getTargetFeatures(feature: Feature): Promise<Feature[]> {
    // Check if the flag depends on other flags.
    // If not, we return only the target flag
    const preFlagIDs = getFeatureIDsDependsOn(feature);
    if (preFlagIDs.length === 0) {
      return [feature];
    }
  
    const prerequisiteFeatures = await this.getPrerequisiteFeaturesFromCache(preFlagIDs);
    return [
      feature,
      ...prerequisiteFeatures,
    ];
  }

@duyhungtnn duyhungtnn requested a review from cre8ivejp March 8, 2025 05:36
Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thank you!

@cre8ivejp cre8ivejp merged commit a92e0b8 into master Mar 10, 2025
5 checks passed
@cre8ivejp cre8ivejp deleted the fix-flag-not-found branch March 10, 2025 00:19
@github-actions github-actions bot mentioned this pull request Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants