Skip to content
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

🪟 🔧 Handle LD feature flags besides featureService.overwrites #19773

Merged
merged 4 commits into from
Dec 10, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,45 @@ import { useAnalyticsService } from "hooks/services/Analytics";
import { useAppMonitoringService, AppActionCodes } from "hooks/services/AppMonitoringService";
import { ExperimentProvider, ExperimentService } from "hooks/services/Experiment";
import type { Experiments } from "hooks/services/Experiment/experiments";
import { FeatureSet, useFeatureService } from "hooks/services/Feature";
import { FeatureSet, FeatureItem, useFeatureService } from "hooks/services/Feature";
import { User } from "packages/cloud/lib/domain/users";
import { useAuthService } from "packages/cloud/services/auth/AuthService";
import { rejectAfter } from "utils/promises";

/**
* This service hardcodes two conventions about the format of the LaunchDarkly feature
* flags we use to override feature settings:
* 1) each feature flag's key (a unique string which is used as the flag's field name in
* LaunchDarkly's JSON payloads) is a string satisfying the LDFeatureName type.
* 2) for each feature flag, LaunchDarkly will return a JSON blob satisfying the
* LDFeatureToggle type.
*
* The primary benefit of programmatically requiring a specific prefix is to provide a
* reliable search term which can be used in LaunchDarkly to filter the list of feature
* flags to all of, and only, the ones which can dynamically toggle features in the UI.
*
* LDFeatureToggle objects can take three forms, representing the three possible decision
* states LaunchDarkly can provide for a user/feature pair:
* |--------------------------+-----------------------------------------------|
* | `{}` | use the application's default feature setting |
* | `{ "enabled": true }` | enable the feature |
* | `{ "enabled": false }` | disable the feature |
* |--------------------------+-----------------------------------------------|
Comment on lines +34 to +38
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a thought, not a necessary blocker for this PR: Would it maybe be nicer if we'd use a string for the tristate, that can either be empty, "true" or "false"? That would be easier to type in LD and would have the benefit, that the default value for string experiments already is the empty string, while using the JSON flag here, we'll require to type {} for each of the new features to not break the code below that destructures the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it would save us a bit of typing, I found the slightly more verbose JSON definitions to be a bit more readable in the devtools network tab. To minimize the hassle of creating new flags, I made sure they can be copy-pasted verbatim from the new documentation in Notion.

That said, "enabled" | "disabled" | "" would have been a very clean type signature ¯\_(ツ)_/¯

*/
const FEATURE_FLAG_PREFIX = "featureService";
type LDFeatureName = `${typeof FEATURE_FLAG_PREFIX}.${FeatureItem}`;
interface LDFeatureToggle {
enabled?: boolean;
}
type LDFeatureFlagResponse = Record<LDFeatureName, LDFeatureToggle>;
type LDInitState = "initializing" | "failed" | "initialized";

/**
* The maximum time in milliseconds we'll wait for LaunchDarkly to finish initialization,
* before running disabling it.
*/
const INITIALIZATION_TIMEOUT = 5000;

const FEATURE_FLAG_EXPERIMENT = "featureService.overwrites";

type LDInitState = "initializing" | "failed" | "initialized";

function mapUserToLDUser(user: User | null, locale: string): LDClient.LDUser {
return user
? {
Expand Down Expand Up @@ -69,18 +93,20 @@ const LDInitializationWrapper: React.FC<React.PropsWithChildren<{ apiKey: string

/**
* Update the feature overwrites based on the LaunchDarkly value.
* It's expected to be a comma separated list of features (the values
* of the enum) that should be enabled. Each can be prefixed with "-"
* to disable the feature instead.
* The feature flag variants which do not include a JSON `enabled` field are filtered
* out; then, each feature corresponding to one of the remaining feature flag overwrites
* is either enabled or disabled for the current user based on the boolean value of its
* overwrite's `enabled` field.
*/
const updateFeatureOverwrites = (featureOverwriteString: string) => {
const featureSet = featureOverwriteString.split(",").reduce((featureSet, featureString) => {
const [key, enabled] = featureString.startsWith("-") ? [featureString.slice(1), false] : [featureString, true];
return {
...featureSet,
[key]: enabled,
};
}, {} as FeatureSet);
const updateFeatureOverwrites = () => {
const allFlags = (ldClient.current?.allFlags() ?? {}) as LDFeatureFlagResponse;
const featureSet: FeatureSet = Object.fromEntries(
Object.entries(allFlags)
.filter(([flagName]) => flagName.startsWith(FEATURE_FLAG_PREFIX))
.map(([flagName, { enabled }]) => [flagName.replace(`${FEATURE_FLAG_PREFIX}.`, ""), enabled])
.filter(([_, enabled]) => typeof enabled !== "undefined")
);

setFeatureOverwrites(featureSet);
};

Expand All @@ -98,7 +124,7 @@ const LDInitializationWrapper: React.FC<React.PropsWithChildren<{ apiKey: string
analyticsService.setContext({ experiments: JSON.stringify(ldClient.current?.allFlags()) });
// Check for overwritten i18n messages
updateI18nMessages();
updateFeatureOverwrites(ldClient.current?.variation(FEATURE_FLAG_EXPERIMENT, ""));
updateFeatureOverwrites();
})
.catch((reason) => {
// If the promise fails, either because LaunchDarkly service fails to initialize, or
Expand All @@ -112,20 +138,13 @@ const LDInitializationWrapper: React.FC<React.PropsWithChildren<{ apiKey: string
});
}

useEffectOnce(() => {
const onFeatureServiceCange = (newOverwrites: string) => {
updateFeatureOverwrites(newOverwrites);
};
ldClient.current?.on(`change:${FEATURE_FLAG_EXPERIMENT}`, onFeatureServiceCange);
return () => ldClient.current?.off(`change:${FEATURE_FLAG_EXPERIMENT}`, onFeatureServiceCange);
});

useEffectOnce(() => {
const onFeatureFlagsChanged = () => {
// Update analytics context whenever a flag changes
analyticsService.setContext({ experiments: JSON.stringify(ldClient.current?.allFlags()) });
// Check for overwritten i18n messages
updateI18nMessages();
updateFeatureOverwrites();
};
ldClient.current?.on("change", onFeatureFlagsChanged);
return () => ldClient.current?.off("change", onFeatureFlagsChanged);
Expand Down