-
Notifications
You must be signed in to change notification settings - Fork 71
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: give option to retrieve toggle definitions with full segment data #574
Conversation
src/repository/index.ts
Outdated
@@ -405,4 +406,18 @@ Message: ${err.message}`, | |||
getToggles(): FeatureInterface[] { | |||
return Object.keys(this.data).map((key) => this.data[key]); | |||
} | |||
|
|||
enhanceWithSegmentData(toggles: FeatureInterface[]): EnhancedFeatureInterface[] { |
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.
There's a lot going on in this method. I think this deserves some unit tests
getFeatureToggleDefinitions(): FeatureInterface[] { | ||
return this.repository.getToggles(); | ||
getFeatureToggleDefinitions( | ||
withFullSegments = false |
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.
This is quite clever from a perspective of not having to muck with the version number but I'm tempted to bite the bullet and go with this signature instead:
getFeatureToggleDefinitions(): EnhancedFeatureInterface[]
It would definitely mean a major version but it'd make the method a lot easier to use. WDYT?
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.
The reason of doing it this way is that segment data can be quite massive (seeing how many userIds some customers have as values.
This is meant to solve a use case but maybe overloading the data should be optional. wdyt?
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.
Actually hadn't thought of that. Yeah fair enough, that's potentially a very, very expensive nested loop so that would be a surprise to anyone who didn't care about this stuff already. Fair enough!
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.
Yeah this looks good but I don't want to mark approve without some tests. This is definitely stuff we'll forget and we likely won't use it ourselves, I'd be worried about accidentally breaking this without noticing in the future
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.
G
This will give the option to retrieve toggle definitions with full segment data
Relates to SR-353
Closes #572