Skip to content

[PM-2282] Make feature flags type safe #8612

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

Merged
merged 13 commits into from
Apr 26, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export class UserKeyRotationService {
request.emergencyAccessKeys = await this.emergencyAccessService.getRotatedKeys(newUserKey);
request.resetPasswordKeys = await this.resetPasswordService.getRotatedKeys(newUserKey);

if (await this.configService.getFeatureFlag<boolean>(FeatureFlag.KeyRotationImprovements)) {
if (await this.configService.getFeatureFlag(FeatureFlag.KeyRotationImprovements)) {
await this.apiService.postUserKeyUpdate(request);
} else {
await this.rotateUserKeyAndEncryptedDataLegacy(request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export class VaultOnboardingComponent implements OnInit, OnChanges, OnDestroy {
) {}

async ngOnInit() {
this.showOnboardingAccess$ = await this.configService.getFeatureFlag$<boolean>(
this.showOnboardingAccess$ = await this.configService.getFeatureFlag$(
FeatureFlag.VaultOnboarding,
false,
);
Expand Down
4 changes: 2 additions & 2 deletions libs/angular/src/directives/if-feature.directive.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Directive, Input, OnInit, TemplateRef, ViewContainerRef } from "@angular/core";

import { FeatureFlag, FeatureFlagValue } from "@bitwarden/common/enums/feature-flag.enum";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";

Expand All @@ -23,7 +23,7 @@ export class IfFeatureDirective implements OnInit {
* Optional value to compare against the value of the feature flag in the config service.
* @default true
*/
@Input() appIfFeatureValue: FeatureFlagValue = true;
@Input() appIfFeatureValue: string | number | boolean = true;

private hasView = false;

Expand Down
30 changes: 28 additions & 2 deletions libs/common/src/enums/feature-flag.enum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,31 @@ export enum FeatureFlag {
AC1795_UpdatedSubscriptionStatusSection = "AC-1795_updated-subscription-status-section",
}

// Replace this with a type safe lookup of the feature flag values in PM-2282
export type FeatureFlagValue = number | string | boolean;
// Map of feature flags to their value type. `string`, `number` and `boolean` are the only supported types.
export const FeatureFlagRuntimeTypes = {
Copy link
Member Author

Choose a reason for hiding this comment

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

While we technically don't need these for this PR, #8561 requires it if we want to introduce a default defaultValue

[FeatureFlag.BrowserFilelessImport]: "boolean",
[FeatureFlag.ItemShare]: "boolean",
[FeatureFlag.FlexibleCollectionsV1]: "boolean",
[FeatureFlag.VaultOnboarding]: "boolean",
[FeatureFlag.GeneratorToolsModernization]: "boolean",
[FeatureFlag.KeyRotationImprovements]: "boolean",
[FeatureFlag.FlexibleCollectionsMigration]: "boolean",
[FeatureFlag.ShowPaymentMethodWarningBanners]: "boolean",
[FeatureFlag.EnableConsolidatedBilling]: "boolean",
[FeatureFlag.AC1795_UpdatedSubscriptionStatusSection]: "boolean",
} as const;

// Helpers for mapping between runtime and static types
type RuntimeType = "boolean" | "string" | "number";
type RuntimeToStatic<T extends RuntimeType> = T extends "boolean"
? boolean
: T extends "string"
? string
: T extends "number"
? number
: never;

// Typescript type of the feature flag values
export type FeatureFlagType<Flag extends FeatureFlag> = RuntimeToStatic<
(typeof FeatureFlagRuntimeTypes)[Flag]
>;
18 changes: 9 additions & 9 deletions libs/common/src/platform/abstractions/config/config.service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Observable } from "rxjs";
import { SemVer } from "semver";

import { FeatureFlag } from "../../../enums/feature-flag.enum";
import { FeatureFlag, FeatureFlagType } from "../../../enums/feature-flag.enum";
import { Region } from "../environment.service";

import { ServerConfig } from "./server-config";
Expand All @@ -17,20 +17,20 @@ export abstract class ConfigService {
* @param defaultValue The default value to return if the feature flag is not set or the server's config is irretrievable
* @returns An observable that emits the value of the feature flag, updates as the server config changes
*/
getFeatureFlag$: <T extends boolean | number | string>(
key: FeatureFlag,
defaultValue?: T,
) => Observable<T>;
getFeatureFlag$: <Flag extends FeatureFlag>(
key: Flag,
defaultValue?: FeatureFlagType<Flag>,
) => Observable<FeatureFlagType<Flag>>;
/**
* Retrieves the value of a feature flag for the currently active user
* @param key The feature flag to retrieve
* @param defaultValue The default value to return if the feature flag is not set or the server's config is irretrievable
* @returns The value of the feature flag
*/
getFeatureFlag: <T extends boolean | number | string>(
key: FeatureFlag,
defaultValue?: T,
) => Promise<T>;
getFeatureFlag: <Flag extends FeatureFlag>(
key: Flag,
defaultValue?: FeatureFlagType<Flag>,
) => Promise<FeatureFlagType<Flag>>;
/**
* Verifies whether the server version meets the minimum required version
* @param minimumRequiredServerVersion The minimum version required
Expand Down
Copy link
Member

@audreyality audreyality Apr 18, 2024

Choose a reason for hiding this comment

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

🤔 Since the default config service has test cases, they should be updated.

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
} from "rxjs";
import { SemVer } from "semver";

import { FeatureFlag, FeatureFlagValue } from "../../../enums/feature-flag.enum";
import { FeatureFlag, FeatureFlagType } from "../../../enums/feature-flag.enum";
import { UserId } from "../../../types/guid";
import { ConfigApiServiceAbstraction } from "../../abstractions/config/config-api.service.abstraction";
import { ConfigService } from "../../abstractions/config/config.service";
Expand Down Expand Up @@ -90,19 +90,20 @@ export class DefaultConfigService implements ConfigService {
map((config) => config?.environment?.cloudRegion ?? Region.US),
);
}
getFeatureFlag$<T extends FeatureFlagValue>(key: FeatureFlag, defaultValue?: T) {

getFeatureFlag$<Flag extends FeatureFlag>(key: Flag, defaultValue?: FeatureFlagType<Flag>) {
return this.serverConfig$.pipe(
map((serverConfig) => {
if (serverConfig?.featureStates == null || serverConfig.featureStates[key] == null) {
return defaultValue;
}

return serverConfig.featureStates[key] as T;
return serverConfig.featureStates[key] as FeatureFlagType<Flag>;
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 suggestion: Since we now have the types available during runtime, maybe we should add some guards here to verify that we actually get the types we expect?

Suggested change
return serverConfig.featureStates[key] as FeatureFlagType<Flag>;
const value = serverConfig.featureStates[key];
if (FeatureFlagRuntimeTypes[key] !== typeof value) { throw new Error("Unknown flag value, possibly undefined?"); } // or maybe return default value?
return value as FeatureFlagType<Flag>;

Or maybe this check should be done when fetching the server config, in which case serverConfig?.featureStates might already be fully type-safe by this point

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planning on using it in my other PR.

}),
);
}

async getFeatureFlag<T extends FeatureFlagValue>(key: FeatureFlag, defaultValue?: T) {
async getFeatureFlag<Flag extends FeatureFlag>(key: Flag, defaultValue?: FeatureFlagType<Flag>) {
return await firstValueFrom(this.getFeatureFlag$(key, defaultValue));
}

Expand Down