Skip to content

Commit

Permalink
Default feature flags as an object (#5810)
Browse files Browse the repository at this point in the history
When an expected flag is not provided by `environment.featureFlags`, the
missing value is taken from a
[string](https://github.com/dfinity/nns-dapp/blob/662509773a569ec966ce7cfcf2ee437ff1f55e9a/frontend/src/lib/constants/environment.constants.ts#L32)
that is supposed to contain all default values. The issue is that this
approach cannot be verified, and it already omits some values, which
could lead to a broken app state when environment constants are missing
or outdated.

In cases where the `featureFlags` environment variable is empty or not
provided, the dapp breaks with a dev console error: `Uncaught (in
promise) Error: Missing mandatory environment variables: featureFlags`
This error is self-explanatory, so no changes are needed to handle this
specific case.

However, when there are missing entries in the environment variable
(e.g., `record { 0="FEATURE_FLAGS"; 1="{}" };`), the dapp also breaks
with the error:`Error: derived() expects stores as input, got a falsy
value.`
This happens because no derived store is created for the missing
value(s). While we could modify the store creation mechanism to always
initialize with the `false` value, I believe having explicitly set
default values is a better solution.

# Changes

- Add `defaultFeatureFlagValues: FeatureFlags<boolean>`.
- Replace missing flags with the entries from
`defaultFeatureFlagValues`.

# Tests

- Added a unit test to verify that the default flags are used.
- Tested manually: reproduced the issue with the current nns-dapp and
missing flags, then confirmed that the updated version works with the
default flags.

# Todos

- [ ] Add entry to changelog (if necessary).
Not necessary.
  • Loading branch information
mstrasinskis authored Nov 22, 2024
1 parent abd2f5a commit 9979071
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 4 deletions.
26 changes: 22 additions & 4 deletions frontend/src/lib/constants/environment.constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,36 @@ export interface FeatureFlags<T> {
TEST_FLAG_EDITABLE: T;
TEST_FLAG_NOT_EDITABLE: T;
}
export const defaultFeatureFlagValues: FeatureFlags<boolean> = {
ENABLE_CKTESTBTC: false,
DISABLE_IMPORT_TOKEN_VALIDATION_FOR_TESTING: false,
ENABLE_PERIODIC_FOLLOWING_CONFIRMATION: false,
ENABLE_EXPORT_NEURONS_REPORT: false,
TEST_FLAG_EDITABLE: false,
TEST_FLAG_NOT_EDITABLE: false,
};

export type FeatureKey = keyof FeatureFlags<boolean>;

const getFeatureFlagsFromEnv = (): FeatureFlags<boolean> => {
let featureFlags = {};
try {
featureFlags = JSON.parse(envVars?.featureFlags);
} catch (e) {
console.error("Error parsing featureFlags", e);
}
// Complement the default flags with the ones from the environment to avoid missing flags.
return { ...defaultFeatureFlagValues, ...featureFlags };
};

/**
* DO NOT USE DIRECTLY
*
* @see feature-flags.store.ts to use feature flags
*/
export const FEATURE_FLAG_ENVIRONMENT: FeatureFlags<boolean> = JSON.parse(
envVars?.featureFlags ??
'{"ENABLE_CKTESTBTC": false, "ENABLE_SNS_TYPES_FILTER": false, "ENABLE_EXPORT_NEURONS_REPORT": false}'
);

export const FEATURE_FLAG_ENVIRONMENT: FeatureFlags<boolean> =
getFeatureFlagsFromEnv();

export const IS_TESTNET: boolean =
DFX_NETWORK !== "mainnet" &&
Expand Down
1 change: 1 addition & 0 deletions frontend/src/tests/lib/api/canisters.api.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ describe("canisters-api", () => {
});

it("should not notify if transfer fails", async () => {
vi.spyOn(console, "error").mockImplementation(() => undefined);
mockLedgerCanister.transfer.mockRejectedValue(new Error());

const call = () =>
Expand Down
53 changes: 53 additions & 0 deletions frontend/src/tests/lib/constants/environment.constants.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { defaultFeatureFlagValues } from "$lib/constants/environment.constants";
import * as envVarsUtils from "$lib/utils/env-vars.utils";

describe("FEATURE_FLAG_ENVIRONMENT", () => {
const environmentVars = envVarsUtils.getEnvVars();

beforeEach(() => {
// The FEATURE_FLAG_ENVIRONMENT is a constant that is set once when the module
// `environment.constants` is imported. To test different states of it,
// we need to reset the imported modules and reimport `environment.constants` for each test.
vi.resetModules();
});

it("should equal the environment values", async () => {
const { FEATURE_FLAG_ENVIRONMENT } = await import(
"$lib/constants/environment.constants"
);
const expectedFlags = JSON.parse(environmentVars.featureFlags);
expect(FEATURE_FLAG_ENVIRONMENT).toEqual(expectedFlags);
});

it("should contain missing entries substituted with default values", async () => {
vi.spyOn(envVarsUtils, "getEnvVars").mockReturnValue({
...environmentVars,
featureFlags: JSON.stringify({}),
});

const { FEATURE_FLAG_ENVIRONMENT } = await import(
"$lib/constants/environment.constants"
);
expect(FEATURE_FLAG_ENVIRONMENT).toEqual(defaultFeatureFlagValues);
});

it("should fallback to default on error", async () => {
const spyConsoleError = vi
.spyOn(console, "error")
.mockImplementation(() => undefined);
vi.spyOn(envVarsUtils, "getEnvVars").mockReturnValue({
...environmentVars,
featureFlags: `{"TEST_FLAG_NOT_EDITABLE": TRUE}`,
});

const { FEATURE_FLAG_ENVIRONMENT } = await import(
"$lib/constants/environment.constants"
);
expect(FEATURE_FLAG_ENVIRONMENT).toEqual(defaultFeatureFlagValues);
expect(spyConsoleError).toBeCalledTimes(1);
expect(spyConsoleError).toBeCalledWith(
"Error parsing featureFlags",
new SyntaxError("Unexpected token T in JSON at position 27")
);
});
});

0 comments on commit 9979071

Please sign in to comment.