feat: Allow users to create type-safe/strictly typed feature flags with useFlags

[ewlsh]

Requirements

• I have added test coverage for new or changed functionality
  • This is typing-only
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Related issues

#139, launchdarkly/js-sdk-common#32

Describe the solution you've provided

This MR updates useFlags generics and definition to allow implementing codebases to specify their own strictly typed feature flag interface.

// Before
declare const useFlags: <T extends LDFlagSet = LDFlagSet>() => T;

// After
declare function useFlags<T extends Record<string, LDFlagValue> = LDFlagSet>(): T;

By declaring useFlags as const and not a function it is not possible to overload its definition. Function overloads allow implementing codebases to re-declare useFlags to be more strict.

Additionally, currently the generic on useFlags is set to extends LDFlagSet, but this is not necessarily true. When useCamelCaseFlagKeys is true, the return value from useFlags can differ from LDFlagSet if LDFlagSet has been augmented for the client.

Describe alternatives you've considered

I also considered introducing a second interface, ReactLDFlagSet (or similar), which did not include an index type but this would not be backwards compatible and it seems like the goal is to have strict typing be opt-in.

Open to other alternatives, in our codebase we've considered writing a wrapping function.

Additional context

Example codebase implementation using these changes:

declare module 'launchdarkly-js-sdk-common' {
    export interface LDFlagSet {
        'show-a-cool-feature': boolean;
        'demonstrate-another-cool-number': number;
    }
}

const ldClient = initialize(
    getClientSideID(),
    { anonymous: true },
    {
        allAttributesPrivate: true,
        sendEvents: true,
    },
);


// ldClient.allFlags()['demonstrate-another-cool-number']

export interface CamelCaseFeatureFlags {
    showACoolFeature: boolean;
    demonstrateAnotherCoolNumber: number;
}

declare module 'launchdarkly-react-client-sdk' {
    export function useFlags(): CamelCaseFeatureFlags;
}

export const LaunchDarklyProvider: React.FC<unknown> = props => {
    return (
        <LDProvider
            clientSideID={getClientSideID()}
            ldClient={ldClient}
            reactOptions={{
                useCamelCaseFlagKeys: true,
            }}
            options={{
                // bootstrap: defaultFeatureFlags,
            }}
        >
            {props.children}
        </LDProvider>
    );
};

[louis-launchdarkly]

Hello @ewlsh, thank you for the contribution! We will discuss the change and give you a reply after that.

[yusinto]

Apologies for the slow response here but there are a couple of build errors.

Please update any to LDFlagValue. Typescript complained about this. Also

Suggested change

	function useFlags<T extends Record<string, any> = LDFlagSet>(): T {
	function useFlags<T extends Record<string, LDFlagValue> = LDFlagSet>(): T {

There is also a prettier issue which should be easy to fix. I will make sure this gets expedited and merged after these are fixed. Thank you.

[ewlsh]

We're still carrying this patch internally, so returning to this with the hopes of upstreaming it.

I've fixed the tslint and prettier issues.

[ewlsh]

@yusinto this is updated.

[kinyoklion]

Hello,

Sorry I am late to looking at this, but I need some help understanding the goals better.

My initial impression is that something like:

export interface CamelCaseFeatureFlags {
  showACoolFeature: boolean;
  demonstrateAnotherCoolNumber: number;
}

export function useCamelCaseFeatureFlags(): CamelCaseFeatureFlags {
  return useFlags<CamelCaseFeatureFlags>();
}

Provides a non-generic hook and doesn't involve modifying the declaration of the underlying package.

Conceptually though I am good with changing to a function, I think it generally should be a function. But I am not comfortable with the typing change.

As modifying the declarations of the library resulting in different types shouldn't maintain an expectation of continued functionality or compatibility. The flags are always an LDFlagSet, which is intended to represent a object indexed by a string with values of type LDValue. If the strings are camel cased or kebab doesn't change that interface, but a customer can of course restrict it further to be a constrained set of strings (via their own hook).

Thank you,
Ryan

[ewlsh]

Hello,

Sorry I am late to looking at this, but I need some help understanding the goals better.

My initial impression is that something like:

export interface CamelCaseFeatureFlags {
  showACoolFeature: boolean;
  demonstrateAnotherCoolNumber: number;
}

export function useCamelCaseFeatureFlags(): CamelCaseFeatureFlags {
  return useFlags<CamelCaseFeatureFlags>();
}

Provides a non-generic hook and doesn't involve modifying the declaration of the underlying package.

Conceptually though I am good with changing to a function, I think it generally should be a function. But I am not comfortable with the typing change.

As modifying the declarations of the library resulting in different types shouldn't maintain an expectation of continued functionality or compatibility. The flags are always an LDFlagSet, which is intended to represent a object indexed by a string with values of type LDValue. If the strings are camel cased or kebab doesn't change that interface, but a customer can of course restrict it further to be a constrained set of strings (via their own hook).

Thank you, Ryan

LDFlagSet

@kinyoklion My understanding is that LDFlagSet can be extended by consumers so that they can declare the key/value of their types. I've definitely seen examples of that in the wild, even if LD doesn't recommend that currently. It helps with autocomplete and type checking. This pattern is also somewhat common in libraries that need to expose configuration like this. Module augmentation and interface merging are supported by TypeScript - though I agree the pattern has pros and cons.

Right now the hook's typing is extends LDFlagSet = LDFlagSet with LDFlatSet as an interface which doesn't make sense if you absolutely don't want that type to be augmented. Defining LDFlagSet as

type LDFlagSet = Record<string, LDFlagValue>
type LDFlagSet = { [key: string]: LDFlagValue }

Would be closer to what you are suggesting, a Record (or inline object type) is what would represent a generic object mapping a string index to a flag value. I think the updated definition I'm providing is simply more flexible. Record<string, LDFlagValue> is the more generic type of LDFlagSet and is compatible with LDFlagSet.

And it allows...

export interface CamelCaseFeatureFlags {
  showACoolFeature: boolean;
  demonstrateAnotherCoolNumber: number;
}

export function useCamelCaseFeatureFlags(): CamelCaseFeatureFlags {
  return useFlags<CamelCaseFeatureFlags>();
}

Lastly LDFlagSet is the return type of allFlags() on the client:

allFlags(): LDFlagSet;

In my mind at least LDFlagSet is conceptually referring to a specific type of object because it is an interface (e.g. "the list of flags returned by the client"). But the type of object returned by useFlags and allFlags can be different if you're using camelCase (and perhaps other options?) - they aren't the same object so the React hook should be bound (extends) by a different or more generic type.