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

Allow users to create type-safe/strictly typed feature flags with useFlags #151

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ewlsh
Copy link

@ewlsh ewlsh commented Aug 30, 2022

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, any> = 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
Copy link
Contributor

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

@yusinto yusinto self-requested a review September 14, 2022 18:46
src/useFlags.ts Outdated
@@ -10,10 +10,10 @@ import context, { ReactSdkContext } from './context';
*
* @return All the feature flags configured in your LaunchDarkly project
*/
const useFlags = <T extends LDFlagSet = LDFlagSet>(): T => {
function useFlags<T extends Record<string, any> = LDFlagSet>(): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 ewlsh force-pushed the ewlsh-modern-health-improve-useflags-types branch 2 times, most recently from 562fb28 to bbf47f9 Compare May 8, 2024 21:19
Convert to function to allow overrides

Closes launchdarkly#139
@ewlsh ewlsh force-pushed the ewlsh-modern-health-improve-useflags-types branch from bbf47f9 to ccf40f4 Compare May 8, 2024 21:21
@ewlsh
Copy link
Author

ewlsh commented May 8, 2024

@yusinto this is updated.

LaunchDarklyReleaseBot pushed a commit that referenced this pull request May 22, 2024
To fix this releaser error when trying to release react sdk 3.3.0:

```bash
stdout >> Installing typedoc
stderr >> npm WARN EBADENGINE Unsupported engine {
stderr >> npm WARN EBADENGINE   package: '@testing-library/[email protected]',
stderr >> npm WARN EBADENGINE   required: { node: '>=18' },
stderr >> npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
stderr >> npm WARN EBADENGINE }
stderr >> npm WARN EBADENGINE Unsupported engine {
stderr >> npm WARN EBADENGINE   package: '@testing-library/[email protected]',
stderr >> npm WARN EBADENGINE   required: { node: '>=18' },
stderr >> npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
stderr >> npm WARN EBADENGINE }
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants