feat: dependent features #520
Conversation
kwasniew
requested review from
thomasheartman,
ivarconr,
FredrikOseberg,
sjaanus and
Tymek
src/client.ts
Outdated
| @@ -55,8 +55,37 @@ export default class UnleashClient extends EventEmitter { | |||
| } | |||
| } | |||
|
|
|||
| isParentDependencySatisfied(feature: FeatureInterface | undefined, context: Context) { | |||
| if (!feature?.name) { | |||
There was a problem hiding this comment.
Can feature not have name?
There was a problem hiding this comment.
this is what the existing type was telling me so I'm respecting it here
There was a problem hiding this comment.
Feature's should have a name, but at least in this SDK we allow people to define their own bootstrapper so it's possible people put goop in there. This one is probably mostly legacy but respecting the existing types seems very sane to me
src/client.ts
Outdated
| return parent.variants.includes(this.getVariant(parent.feature, context).name); | ||
| } | ||
|
|
||
| return parent.enabled !== false |
There was a problem hiding this comment.
Can we make it more readable, and not do negate operator, but instead ===
There was a problem hiding this comment.
by default enabled is not provided so it's undefined. enabled: false is a special case for kill-switches in parent flag.This code is explicit about explicit enabled false and everything else.
There was a problem hiding this comment.
I reorganized the conditions a bit but the !== false has to stay as it is
src/client.ts
Outdated
| return feature.dependencies.every(parent => { | ||
| const parentFeature = this.repository.getToggle(parent.feature); | ||
|
|
||
| if (parentFeature?.dependencies?.length) { |
There was a problem hiding this comment.
Can we make it more readable, Does this mean length exists or length === 0?
There was a problem hiding this comment.
nope, it should also cater for undefined dependencies. Setting === 0 breaks the spec
There was a problem hiding this comment.
move it to precondition check though
src/client.ts
Outdated
| @@ -55,8 +55,32 @@ export default class UnleashClient extends EventEmitter { | |||
| } | |||
| } | |||
|
|
|||
| isParentDependencySatisfied(feature: FeatureInterface | undefined, context: Context) { | |||
| if (!feature?.name || !feature.dependencies?.length) { | |||
There was a problem hiding this comment.
What's with the name check here? Name should always be a non empty string, do I have the dumb or is the same as
if (!feature?.dependencies?.length) {
return true;
}
There was a problem hiding this comment.
still passes the spec. not sure why I did the name check but it's not in the type anymore. I'm hallucinating :D
There was a problem hiding this comment.
Glancing over the metrics code, it looks correct but I think it's worth having one or two tests to check that the parent doesn't get metric counts (I'm guessing, don't know if that's what you want). Especially if this is going to be the reference implementation so others know what to do, since the spec doesn't handle that
|
@sighphyre addressed all your points :) |
About the changes
Dependent features implementation based on spec: Unleash/client-specification#63
Extra checks that may also be covered by other SDKs (listing them since this is our reference implementation):
Important files
Discussion points