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

[SDKS-7830] flag sets #44

Merged
merged 2 commits into from
Dec 18, 2023
Merged

[SDKS-7830] flag sets #44

merged 2 commits into from
Dec 18, 2023

Conversation

emmaz90
Copy link
Contributor

@emmaz90 emmaz90 commented Dec 13, 2023

Angular SDK plugin

What did you accomplish?

  • Updated splitio-browserjs to 0.12.0
  • Added the following methods regarding flag sets:
    • getTreatmentsByFlagSet
    • getTreatmentsWithConfigByFlagSet
    • getTreatmentsByFlagSets
    • getTreatmentsWithConfigByFlagSets

How do we test the changes introduced in this PR?

Extra Notes

CHANGES.txt Outdated
@@ -1,5 +1,6 @@
2.0.0 (Dec XX, 2023)
- Updated the minimum Angular version to match Angular's support up to date. Breaking change version is regarding the Angular minimum version update, there are no breaking changes to Split's plugin API or functionality itself.
- Updated @splitsoftware/splitio-browserjs package to version 0.12.0

Choose a reason for hiding this comment

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

Update to version 0.13.0

@@ -1,5 +1,6 @@
2.0.0 (Dec XX, 2023)
- Updated the minimum Angular version to match Angular's support up to date. Breaking change version is regarding the Angular minimum version update, there are no breaking changes to Split's plugin API or functionality itself.
Copy link

@EmilianoSanchez EmilianoSanchez Dec 14, 2023

Choose a reason for hiding this comment

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

I would check other CHANGES.txt files if there is a format for breaking change, something like: - BREAKING CHANGE: Updated ....

@@ -1,5 +1,6 @@
2.0.0 (Dec XX, 2023)
Copy link

@EmilianoSanchez EmilianoSanchez Dec 14, 2023

Choose a reason for hiding this comment

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

Don't forget to add an item for flag sets (either here or in a different PR). Also other updates like defaultTreatment in SplitView, etc.

* @param {Attributes=} attributes - An object of type Attributes defining the attributes for the given key.
* @returns {Treatments} - The treatments object map.
*/
getTreatmentsByFlagSet(key: SplitIO.SplitKey, flagSetName: string, attributes?: SplitIO.Attributes | undefined): SplitIO.Treatments;

Choose a reason for hiding this comment

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

attributes?: SplitIO.Attributes | undefined -> attributes?: SplitIO.Attributes (undefined is redundant if using ?).
Check other method signatures too.

@@ -33,6 +33,10 @@ export const CONTROL_CLIENT = {
});
return result;
},
getTreatmentsByFlagSet: () => { return []; },

Choose a reason for hiding this comment

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

This should return {}. Some test should be validating it.
TDD: a test should be failing, and after the fix the test should pass.

@@ -6,7 +6,9 @@ export const mockedFeatureFlagView = [
'name':
'test_split',
'trafficType': 'localhost',
'treatments': ['on']
'treatments': ['on'],
'defaultTreatment': 'control',

Choose a reason for hiding this comment

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

This is just a mock, but defaultTreatment cannot be 'control'. It is one of the items in treatments list.

@emmaz90 emmaz90 merged commit 682f485 into development Dec 18, 2023
2 checks passed
@emmaz90 emmaz90 deleted the sdks-7830 branch December 18, 2023 18:04
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