-
-
Notifications
You must be signed in to change notification settings - Fork 658
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: feature admin API returns dependencies and children #4848
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
export interface IDependentFeaturesReadModel { | ||
getChildren(parent: string): Promise<string[]>; | ||
getParents(child: string): Promise<string[]>; | ||
getParents(child: string): Promise<IDependency[]>; |
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.
currently we only need string but I want to have API that will be prepared for the upcoming enhancements (variants, enabled/disabled)
return rows.map((row) => ({ | ||
feature: row.parent, | ||
enabled: row.enabled, | ||
variants: row.variants, |
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.
I was surprised that JSON.parse is not needed here
@@ -157,6 +160,8 @@ class FeatureToggleService { | |||
|
|||
private privateProjectChecker: IPrivateProjectChecker; | |||
|
|||
private dependentFeaturesReadModel: IDependentFeaturesReadModel; |
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.
we only depend on read operations in a different module
if (projectId) { | ||
await this.validateFeatureBelongsToProject({ | ||
featureName, | ||
projectId, | ||
}); | ||
} | ||
|
||
let dependencies: IDependency[] = []; | ||
let children: string[] = []; | ||
if (this.flagResolver.isEnabled('dependentFeatures')) { |
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.
here we do optional enhancement with parent/child information
@@ -3394,3 +3395,29 @@ test('should not be allowed to update with invalid strategy type name', async () | |||
400, | |||
); | |||
}); | |||
|
|||
test('should list dependencies and children', async () => { |
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 shows how this new feature works
const child = uuidv4(); | ||
await app.createFeature(parent, 'default'); | ||
await app.createFeature(child, 'default'); | ||
await app.addDependency(child, parent); |
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 new method since it may come in handy and also code is symmetrical when we do everything with app.doXYZ
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.
LGTM
About the changes
Admin feature API returns dependencies and children
Details:
Important files
Discussion points
I decided not to touch the big features_view for now. Reasons: it's already difficult to reason about + for common parents it would generate lots of new rows