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 @@ -183,7 +183,7 @@ class FilelessImporterBackground implements FilelessImporterBackgroundInterface
return;
}

const filelessImportFeatureFlagEnabled = await this.configService.getFeatureFlag<boolean>(
const filelessImportFeatureFlagEnabled = await this.configService.getFeatureFlag(
FeatureFlag.BrowserFilelessImport,
);
const userAuthStatus = await this.authService.getAuthStatus();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ export class OrganizationLayoutComponent implements OnInit, OnDestroy {

protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
FeatureFlag.ShowPaymentMethodWarningBanners,
false,
);

constructor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,6 @@ export class MemberDialogComponent implements OnDestroy {
groups: groups$,
flexibleCollectionsV1Enabled: this.configService.getFeatureFlag$(
FeatureFlag.FlexibleCollectionsV1,
false,
),
})
.pipe(takeUntil(this.destroy$))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,10 @@ export class AccountComponent {

protected flexibleCollectionsMigrationEnabled$ = this.configService.getFeatureFlag$(
FeatureFlag.FlexibleCollectionsMigration,
false,
);

flexibleCollectionsV1Enabled$ = this.configService.getFeatureFlag$(
FeatureFlag.FlexibleCollectionsV1,
false,
);

// FormGroup validators taken from server Organization domain object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,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 @@ -84,7 +84,6 @@ export class OrganizationSubscriptionCloudComponent implements OnInit, OnDestroy

this.showUpdatedSubscriptionStatusSection$ = this.configService.getFeatureFlag$(
FeatureFlag.AC1795_UpdatedSubscriptionStatusSection,
false,
);
}

Expand Down
1 change: 0 additions & 1 deletion apps/web/src/app/layouts/user-layout.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export class UserLayoutComponent implements OnInit {

protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
FeatureFlag.ShowPaymentMethodWarningBanners,
false,
);

constructor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export enum CollectionDialogAction {
})
export class CollectionDialogComponent implements OnInit, OnDestroy {
protected flexibleCollectionsV1Enabled$ = this.configService
.getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1, false)
.getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1)
.pipe(first());

private destroy$ = new Subject<void>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export class BulkDeleteDialogComponent {

private flexibleCollectionsV1Enabled$ = this.configService.getFeatureFlag$(
FeatureFlag.FlexibleCollectionsV1,
false,
);

constructor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,8 @@ 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,
);
this.onboardingTasks$ = this.vaultOnboardingService.vaultOnboardingState$;
await this.setOnboardingTasks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ export class VaultComponent implements OnInit, OnDestroy {
protected currentSearchText$: Observable<string>;
protected flexibleCollectionsV1Enabled$ = this.configService.getFeatureFlag$(
FeatureFlag.FlexibleCollectionsV1,
false,
);

private searchText$ = new Subject<string>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export class AttachmentsComponent extends BaseAttachmentsComponent implements On
async ngOnInit() {
await super.ngOnInit();
this.flexibleCollectionsV1Enabled = await firstValueFrom(
this.configService.getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1, false),
this.configService.getFeatureFlag$(FeatureFlag.FlexibleCollectionsV1),
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@
) {}

async ngOnInit() {
const v1FCEnabled = await this.configService.getFeatureFlag(
FeatureFlag.FlexibleCollectionsV1,
false,
);
const v1FCEnabled = await this.configService.getFeatureFlag(FeatureFlag.FlexibleCollectionsV1);

Check warning on line 73 in apps/web/src/app/vault/org-vault/bulk-collection-assignment-dialog/bulk-collection-assignment-dialog.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/vault/org-vault/bulk-collection-assignment-dialog/bulk-collection-assignment-dialog.component.ts#L73

Added line #L73 was not covered by tests
const org = await this.organizationService.get(this.params.organizationId);

if (org.canEditAllCiphers(v1FCEnabled)) {
Expand Down
1 change: 0 additions & 1 deletion apps/web/src/app/vault/org-vault/vault.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@ export class VaultComponent implements OnInit, OnDestroy {

this._flexibleCollectionsV1FlagEnabled = await this.configService.getFeatureFlag(
FeatureFlag.FlexibleCollectionsV1,
false,
);

const filter$ = this.routedVaultFilterService.filter$;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export class ClientsComponent implements OnInit {

protected enableConsolidatedBilling$ = this.configService.getFeatureFlag$(
FeatureFlag.EnableConsolidatedBilling,
false,
);
private destroy$ = new Subject<void>();
private _searchText$ = new BehaviorSubject<string>("");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ export class ProvidersLayoutComponent {

protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
FeatureFlag.ShowPaymentMethodWarningBanners,
false,
);

protected enableConsolidatedBilling$ = this.configService.getFeatureFlag$(
FeatureFlag.EnableConsolidatedBilling,
false,
);

constructor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export class AccountComponent {

protected enableDeleteProvider$ = this.configService.getFeatureFlag$(
FeatureFlag.EnableDeleteProvider,
false,
);

constructor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,10 @@ export class SetupComponent implements OnInit {

protected showPaymentMethodWarningBanners$ = this.configService.getFeatureFlag$(
FeatureFlag.ShowPaymentMethodWarningBanners,
false,
);

protected enableConsolidatedBilling$ = this.configService.getFeatureFlag$(
FeatureFlag.EnableConsolidatedBilling,
false,
);

constructor(
Expand Down
8 changes: 3 additions & 5 deletions libs/angular/src/directives/if-feature.directive.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { ComponentFixture, TestBed } from "@angular/core/testing";
import { By } from "@angular/platform-browser";
import { mock, MockProxy } from "jest-mock-extended";

import { FeatureFlag, FeatureFlagValue } from "@bitwarden/common/enums/feature-flag.enum";
import { AllowedFeatureFlagTypes, 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 Down Expand Up @@ -41,10 +41,8 @@ describe("IfFeatureDirective", () => {
let content: HTMLElement;
let mockConfigService: MockProxy<ConfigService>;

const mockConfigFlagValue = (flag: FeatureFlag, flagValue: FeatureFlagValue) => {
mockConfigService.getFeatureFlag.mockImplementation((f, defaultValue) =>
flag == f ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
);
const mockConfigFlagValue = (flag: FeatureFlag, flagValue: AllowedFeatureFlagTypes) => {
mockConfigService.getFeatureFlag.mockImplementation((f) => Promise.resolve(flagValue as any));
};

const queryContent = (testId: string) =>
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 { AllowedFeatureFlagTypes, 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: AllowedFeatureFlagTypes = true;

private hasView = false;

Expand Down
8 changes: 4 additions & 4 deletions libs/angular/src/platform/guard/feature-flag.guard.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ describe("canAccessFeature", () => {
flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
);
} else if (typeof flagValue === "string") {
mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = "") =>
flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
mockConfigService.getFeatureFlag.mockImplementation((flag) =>
flag == testFlag ? Promise.resolve(flagValue as any) : Promise.resolve(""),
);
} else if (typeof flagValue === "number") {
mockConfigService.getFeatureFlag.mockImplementation((flag, defaultValue = 0) =>
flag == testFlag ? Promise.resolve(flagValue) : Promise.resolve(defaultValue),
mockConfigService.getFeatureFlag.mockImplementation((flag) =>
flag == testFlag ? Promise.resolve(flagValue as any) : Promise.resolve(0),
);
}

Expand Down
1 change: 0 additions & 1 deletion libs/angular/src/vault/components/add-edit.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ export class AddEditComponent implements OnInit, OnDestroy {
async ngOnInit() {
this.flexibleCollectionsV1Enabled = await this.configService.getFeatureFlag(
FeatureFlag.FlexibleCollectionsV1,
false,
);
this.writeableCollections = await this.loadCollections();
this.canUseReprompt = await this.passwordRepromptService.enabled();
Expand Down
36 changes: 34 additions & 2 deletions libs/common/src/enums/feature-flag.enum.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* Feature flags.
*
* Flags MUST be short lived and SHALL be removed once enabled.
*/
export enum FeatureFlag {
BrowserFilelessImport = "browser-fileless-import",
ItemShare = "item-share",
Expand All @@ -13,5 +18,32 @@ export enum FeatureFlag {
EnableDeleteProvider = "AC-1218-delete-provider",
}

// Replace this with a type safe lookup of the feature flag values in PM-2282
export type FeatureFlagValue = number | string | boolean;
export type AllowedFeatureFlagTypes = boolean | number | string;

// Helper to ensure the value is treated as a boolean.
const FALSE = false as boolean;

/**
* Default value for feature flags.
*
* DO NOT enable previously disabled flags, REMOVE them instead.
* We support true as a value as we prefer flags to "enable" not "disable".
*/
export const DefaultFeatureFlagValue = {
[FeatureFlag.BrowserFilelessImport]: FALSE,
[FeatureFlag.ItemShare]: FALSE,
[FeatureFlag.FlexibleCollectionsV1]: FALSE,
[FeatureFlag.VaultOnboarding]: FALSE,
[FeatureFlag.GeneratorToolsModernization]: FALSE,
[FeatureFlag.KeyRotationImprovements]: FALSE,
[FeatureFlag.FlexibleCollectionsMigration]: FALSE,
[FeatureFlag.ShowPaymentMethodWarningBanners]: FALSE,
[FeatureFlag.EnableConsolidatedBilling]: FALSE,
[FeatureFlag.AC1795_UpdatedSubscriptionStatusSection]: FALSE,
[FeatureFlag.UnassignedItemsBanner]: FALSE,
[FeatureFlag.EnableDeleteProvider]: FALSE,
} satisfies Record<FeatureFlag, AllowedFeatureFlagTypes>;

export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue;

export type FeatureFlagValueType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];
14 changes: 3 additions & 11 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, FeatureFlagValueType } from "../../../enums/feature-flag.enum";
import { Region } from "../environment.service";

import { ServerConfig } from "./server-config";
Expand All @@ -14,23 +14,15 @@ export abstract class ConfigService {
/**
* 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 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) => Observable<FeatureFlagValueType<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) => Promise<FeatureFlagValueType<Flag>>;
/**
* Verifies whether the server version meets the minimum required version
* @param minimumRequiredServerVersion The minimum version required
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Jsonify } from "type-fest";

import { AllowedFeatureFlagTypes } from "../../../enums/feature-flag.enum";
import {
ServerConfigData,
ThirdPartyServerConfigData,
Expand All @@ -14,7 +15,7 @@ export class ServerConfig {
server?: ThirdPartyServerConfigData;
environment?: EnvironmentServerConfigData;
utcDate: Date;
featureStates: { [key: string]: string } = {};
featureStates: { [key: string]: AllowedFeatureFlagTypes } = {};

constructor(serverConfigData: ServerConfigData) {
this.version = serverConfigData.version;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Jsonify } from "type-fest";

import { AllowedFeatureFlagTypes } from "../../../enums/feature-flag.enum";
import { Region } from "../../abstractions/environment.service";
import {
ServerConfigResponse,
Expand All @@ -13,7 +14,7 @@ export class ServerConfigData {
server?: ThirdPartyServerConfigData;
environment?: EnvironmentServerConfigData;
utcDate: string;
featureStates: { [key: string]: string } = {};
featureStates: { [key: string]: AllowedFeatureFlagTypes } = {};

constructor(serverConfigResponse: Partial<ServerConfigResponse>) {
this.version = serverConfigResponse?.version;
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,11 @@
} from "rxjs";
import { SemVer } from "semver";

import { FeatureFlag, FeatureFlagValue } from "../../../enums/feature-flag.enum";
import {
DefaultFeatureFlagValue,
FeatureFlag,
FeatureFlagValueType,
} 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 @@ -89,20 +93,21 @@
map((config) => config?.environment?.cloudRegion ?? Region.US),
);
}
getFeatureFlag$<T extends FeatureFlagValue>(key: FeatureFlag, defaultValue?: T) {

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

Check warning on line 101 in libs/common/src/platform/services/config/default-config.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/platform/services/config/default-config.service.ts#L101

Added line #L101 was not covered by tests
}

return serverConfig.featureStates[key] as T;
return serverConfig.featureStates[key] as FeatureFlagValueType<Flag>;

Check warning on line 104 in libs/common/src/platform/services/config/default-config.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/platform/services/config/default-config.service.ts#L104

Added line #L104 was not covered by tests
}),
);
}

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

Check warning on line 110 in libs/common/src/platform/services/config/default-config.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/platform/services/config/default-config.service.ts#L110

Added line #L110 was not covered by tests
}

checkServerMeetsVersionRequirement$(minimumRequiredServerVersion: SemVer) {
Expand Down
Loading