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

Hinton · 2024-04-04T13:13:28Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Refactors the feature flags in ConfigService to be type safe. It also moves the default value to a centralized location rather than the caller defining it. This ensures consistency across the various places they are used.

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team
Ensure that all UI additions follow WCAG AA requirements

github-actions · 2024-04-04T13:51:38Z

Checkmarx One – Scan Summary & Details – e931223c-b01c-473e-881c-3995962525bf

No New Or Fixed Issues Found

Hinton · 2024-04-04T15:24:39Z

libs/common/src/enums/feature-flag.enum.ts

-// 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 = {


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

codecov · 2024-04-04T16:23:47Z

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 27.70%. Comparing base (fffef95) to head (1b18c78).

Files	Patch %	Lines
...platform/services/config/default-config.service.ts	25.00%	3 Missing ⚠️
...log/bulk-collection-assignment-dialog.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8612      +/-   ##
==========================================
+ Coverage   27.67%   27.70%   +0.02%     
==========================================
  Files        2369     2369              
  Lines       69049    69052       +3     
  Branches    12919    12919              
==========================================
+ Hits        19112    19128      +16     
+ Misses      48504    48491      -13     
  Partials     1433     1433

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coroiu

Can't really comment on libs/common/src/enums/feature-flag.enum.ts since it's my own code but added some comments in other parts

libs/angular/src/directives/if-feature.directive.ts

libs/angular/src/directives/if-feature.directive.spec.ts

coroiu · 2024-04-05T07:28:47Z

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

    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>;


🎨 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

I was planning on using it in my other PR.

libs/common/src/enums/feature-flag.enum.ts

audreyality

👍🏻 kudos!
🤔 non-blocking concern
⛏️ nitpick

audreyality · 2024-04-18T14:44:10Z

libs/common/src/enums/feature-flag.enum.ts

+
+export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue;
+
+export type FeatureFlagType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];


Suggested change

export type FeatureFlagType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];

/** gets the type of a `FeatureFlag` value. */

export type FeatureFlagType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];

libs/common/src/enums/feature-flag.enum.ts

libs/common/src/platform/abstractions/config/config.service.ts

audreyality · 2024-04-18T14:53:32Z

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

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

…ypesafe-feature-flags

Make feature flags type safe

e6a6319

Hinton requested review from a team as code owners April 4, 2024 13:13

Hinton requested review from coroiu, shane-melton and jlf0dev April 4, 2024 13:13

Hinton marked this pull request as draft April 4, 2024 13:14

github-actions bot added the needs-qa Marks a PR as requiring QA approval label Apr 4, 2024

Also provide runtime types

87eb7e3

Hinton marked this pull request as ready for review April 4, 2024 15:23

Hinton commented Apr 4, 2024

View reviewed changes

Fix test types

31f6a5c

Hinton removed the needs-qa Marks a PR as requiring QA approval label Apr 4, 2024

Hinton and others added 2 commits April 4, 2024 18:34

Merge branch 'main' into ps/PM-2282-typesafe-feature-flags

1877773

Remove another explicit type

31a3da0

Hinton requested a review from a team as a code owner April 4, 2024 16:59

coroiu reviewed Apr 5, 2024

View reviewed changes

Move default value to static definition

31135c0

Hinton requested review from a team as code owners April 5, 2024 12:39

Hinton requested a review from r-tome April 5, 2024 12:39

Change flag from test value

926e451

coroiu reviewed Apr 5, 2024

View reviewed changes

libs/common/src/enums/feature-flag.enum.ts Outdated Show resolved Hide resolved

Hinton added 4 commits April 5, 2024 14:53

Fix type errors

1434cdc

Change to aliased FALSE

734eda4

Fix test

835a8a2

Update test

ae94ee5

Hinton requested a review from MGibson1 April 15, 2024 13:00

audreyality previously approved these changes Apr 18, 2024

View reviewed changes

r-tome previously approved these changes Apr 19, 2024

View reviewed changes

Hinton added 2 commits April 19, 2024 17:21

Merge branch 'main' of github.com:bitwarden/clients into ps/PM-2282-t…

46a51ac

…ypesafe-feature-flags

Resolve review feedback

1b18c78

Hinton dismissed stale reviews from r-tome and audreyality via 1b18c78 April 19, 2024 15:35

MGibson1 approved these changes Apr 19, 2024

View reviewed changes

audreyality approved these changes Apr 19, 2024

View reviewed changes

jlf0dev approved these changes Apr 23, 2024

View reviewed changes

cturnbull-bitwarden approved these changes Apr 23, 2024

View reviewed changes

r-tome approved these changes Apr 24, 2024

View reviewed changes

shane-melton approved these changes Apr 25, 2024

View reviewed changes

Hinton merged commit 14b2eb9 into main Apr 26, 2024
63 checks passed

Hinton deleted the ps/PM-2282-typesafe-feature-flags branch April 26, 2024 10:57

justindbaur mentioned this pull request Apr 26, 2024

Fix ConfigService Merge Conflict #8932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

Hinton commented Apr 4, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Apr 4, 2024 •

edited

Loading

Uh oh!

Hinton Apr 4, 2024

Uh oh!

codecov bot commented Apr 4, 2024 •

edited

Loading

Uh oh!

coroiu left a comment

Uh oh!

Uh oh!

Uh oh!

coroiu Apr 5, 2024

Uh oh!

Hinton Apr 5, 2024

Uh oh!

Uh oh!

audreyality left a comment •

edited

Loading

Uh oh!

audreyality Apr 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

audreyality Apr 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

-        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>;


		export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue;

		export type FeatureFlagType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];

	export type FeatureFlagType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];
	/** gets the type of a `FeatureFlag` value. */
	export type FeatureFlagType<Flag extends FeatureFlag> = DefaultFeatureFlagValueType[Flag];

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

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

Uh oh!

Conversation

Hinton commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Objective

Before you submit

Uh oh!

github-actions bot commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No New Or Fixed Issues Found

Uh oh!

Hinton Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coroiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coroiu Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

Hinton Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

audreyality left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

audreyality Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

audreyality Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Hinton commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

audreyality left a comment •

edited

Loading

audreyality Apr 18, 2024 •

edited

Loading