fix: feature flag playground features in new store #5013
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| if (this.flagResolver.isEnabled('separateAdminClientApi')) { | ||
| result = (await this.getFeatureToggles( | ||
| query, | ||
| )) as IFeatureToggleClient[]; |
There was a problem hiding this comment.
will clean this up when we remove the feature flag
There was a problem hiding this comment.
shouldn't we do one query or the other? Here we're fetching getPlayground first and then override it with another query
There was a problem hiding this comment.
If we wanted to optimise this maximally, we could do that. But since this is the admin API and this feature flag should be removed asap to avoid holding all this context, I think it's fine.
FredrikOseberg
force-pushed
the
refactor/move-playground
branch
from
549ecc6
to
5d17f6a
Compare
| export const buildPlaygroundFeaturesFromRows = ( | ||
| rows: any[], | ||
| dependentFeaturesEnabled: boolean, | ||
| featureQuery?: IFeatureToggleQuery, | ||
| ): FeatureConfigurationClient[] => { | ||
| let result = rows.reduce((acc, r) => { | ||
| const feature: PartialDeep<IFeatureToggleClient> = acc[r.name] ?? { | ||
| strategies: [], | ||
| }; | ||
| if (isUnseenStrategyRow(feature, r) && !r.strategy_disabled) { | ||
| feature.strategies?.push(rowToStrategy(r)); | ||
| } | ||
| if (isNewTag(feature, r)) { | ||
| addTag(feature, r); | ||
| } | ||
| if (featureQuery?.inlineSegmentConstraints && r.segment_id) { | ||
| addSegmentToStrategy(feature, r); | ||
| } else if (!featureQuery?.inlineSegmentConstraints && r.segment_id) { | ||
| addSegmentIdsToStrategy(feature, r); |
There was a problem hiding this comment.
Will clean up this duplication in a follow-up PR
FredrikOseberg
force-pushed
the
refactor/move-playground
branch
from
5971dfe
to
550ca8c
Compare
| @@ -36,6 +37,10 @@ export interface IFeatureToggleStore extends Store<FeatureToggle, string> { | |||
| userId?: number, | |||
| archived?: boolean, | |||
| ): Promise<FeatureToggle[]>; | |||
| getPlaygroundFeatures( | |||
| dependentFeaturesEnabled: boolean, | |||
There was a problem hiding this comment.
why are we passing it as args instead of passing flag in a constructor as we did before in the previous client store?
There was a problem hiding this comment.
I didn't want to introduce it in the feature toggle store, because the feature toggle store does not currently receive the config. I decided to go this route for now, since I expect the feature flag to be removed shortly.
| valueB: any; | ||
| } | ||
|
|
||
| export function deepDiff(arr1: any[], arr2: any[]): Difference[] | null { |
There was a problem hiding this comment.
I don't see this function in the old code. Is it new?
There was a problem hiding this comment.
does it have tests? what's the advantage over using lodash isEqual?
There was a problem hiding this comment.
I should have noted that I plan to follow up with some more test coverage in the next PR. The advantage is that it will actually list the two objects that differ, so that you can see what is the difference instead of just getting a boolean value.
Have playground use the method in the feature-toggle-service instead of asking the client-feature-toggle-store