launchdarkly / js-sdk-common

Code shared between all LaunchDarkly client-side JS-based SDKs
Other
3 stars 26 forks source link

Type-safe feature flags #32

Open glentakahashi opened 3 years ago

glentakahashi commented 3 years ago

Is your feature request related to a problem? Please describe. We currently do this in our code:

launchdarkly-js-common-sdk.d.ts :

declare module "launchdarkly-js-sdk-common" {
  // eslint-disable-next-line @typescript-eslint/no-empty-interface
  export interface LDFlagSet {
    someFlag: boolean;
    anotherFlag: boolean;
  }
}

Which allows us to use interface merging in order to get autocompletion on feature flags.

However, since it only extends the existing interface ([key: string]: LDFlagValue), it's still just as easy to accidentally use a nonexistent flag in the code as any key returned is any. This means that when deleting a feature flag, its relatively easy to accidentally leave behind parts of code that still reference this key, as the typing still resolves to any.

Describe the solution you'd like I would like there to be a convenient way to get type safety for feature flags by defining an override somewhere in the code. The best case scenario for this would be for LDFlagSet to be an empty interface by default, however that's obviously a huge breaking change. If that were the case though, it would make it so that we can strongly type our feature flags by doing the above declaration merging.

Describe alternatives you've considered We've tried doing more complex typescript type resolution by excluding the builtin types and using our own, but I was never able to get it to work properly. There may be a way, but there a number of outstanding typescript issues here as well that don't give me hope e.g. https://github.com/microsoft/TypeScript/issues/36146

eli-darkly commented 3 years ago

I can't imagine how this could possibly work. It might help if you could provide some kind of example of what you'd like your code to look like— I mean, not just how you would access the flag, but how you would "define an override somewhere in the code" to enforce this typing. I also can't tell if you're actually talking about typing as in the data type of the flag value (which can't really be enforced in code, since you can change the variation values to any type on the service side), or just the existence of a specific flag key (even though you can't prevent a flag from being deleted on the service side either).

glentakahashi commented 3 years ago

So the above works out of the box for us with typescript by defining the file above in the root project directory. An example in vscode:

Screen Shot 2020-11-09 at 5 55 55 PM Screen Shot 2020-11-09 at 5 57 00 PM Screen Shot 2020-11-09 at 5 57 06 PM

You're right in that it doesn't fully protect, since the service and code can get out of sync, but it does greatly improve the out of the box experience. This keeps all of our feature flag definitions in a single file, so the manual effort to keep things in sync is greatly decreased.

This doesn't prevent the following though (fake flag is typed as any and doesn't create compiler errors):

Screen Shot 2020-11-09 at 6 00 00 PM

As an alternative, what we ended up doing was this:

Screen Shot 2020-11-09 at 6 00 26 PM

and then we enabled the eslint rules:

Screen Shot 2020-11-09 at 6 00 46 PM

Which works decently well, but would still prefer a different method that happens within ts native instead of enforced by eslint rules

eli-darkly commented 3 years ago

I'm sorry, I'm having trouble following your example - it seems like the code sample you posted in the PR description doesn't match what you're saying in the most recent comment, so I'm not sure what you meant by "the above works out of the box for us with typescript by defining the file above". You've provided two different versions of launchdarkly-js-sdk-common.d.ts, and no example of what's in FeatureFlagSet.

glentakahashi commented 3 years ago

Oh sorry, i missed a file, FeatureFlagSet is just:

export interface FeatureFlagSet {
  "input-update-on-enter": boolean;
  "multiplayer-test-page": boolean;
  multiplayer: boolean;
  "data-connections": boolean;
  comments: boolean;
  "cache-default-app-state": boolean;
  "story-mode": boolean;
  notifications: boolean;
  runButtonInput: boolean;
  "single-monaco": boolean;
  "org-migration": boolean;
}

So restating my post in a more concise way:

We can get auto-completion for useFlags by adding an additional type definition for launchdarkly-js-sdk-common.d.ts in our src folder, and using declaration merging (https://www.typescriptlang.org/docs/handbook/declaration-merging.html#merging-interfaces) on the interface. However, declaration merging is purely additive, and still allows any other feature flag to be returned from useFlags with the type of any. It would be ideal if there were a way this could be even more typesafe, where referring to a missing feature flag in useFlags would return an undefined error instead.

As an alternative, we have just been using eslint rules to disallow using useFlags directly, and instead created our own useHexFlags that just casts useFlags using our typed feature flag files.

codeBelt commented 3 years ago

I would also like to useFlag to be typeable. You could do something like:

export function useFlags<F extends LDFlagSet>(): F {
  ...
}

This way you can do:

export interface MyFlags {
  addServicesRevenueDialog: boolean;
}

...

export const MyComponent: React.FC<IProps> = (props) => {
  const flags = useFlags<MyFlags>();

  // TypeScript knowns about "addServicesRevenueDialog"
  flags.addServicesRevenueDialog

  return ...
}

I would also suggest updating LDFlagValue to something like:

-export type LDFlagValue = any;

+export type LDFlagValue = boolean | number | string | undefined;
eli-darkly commented 3 years ago

Regarding the type definition of LDFlagValue, please see https://github.com/launchdarkly/js-sdk-common/issues/16.

mellis481 commented 2 years ago

+1 for the desire to make flags type-safe.

odell-allstripes commented 2 years ago

+1 on this issue. In particular the use of an index signature with a value of any is difficult to correct for. Since TS interfaces don't let you subtract an index signature, you can't supply a more specific type in your own declaration file.

Something like this should work but it doesn't, as all other string keys still have an any value on the interface:

declare module 'launchdarkly-js-sdk-common' {
  export interface LDFlagSet {
    testFlag: boolean,
  }
}

I agree re making useFlags hook generic. It's fine if the type defaults to the flexible LDFlagSet but that should not be hardcoded as the return value in LD module's useFlags.d.ts.

TNonet commented 1 year ago

Is there any update on this request?

nieblara commented 1 year ago

Also looking for an update on this request! Thanks!

josephearl commented 11 months ago

IMO a better solution here would be to add typesafe methods to LDClient like the Java client such as boolVariation, stringVariation etc that only accept default values of the correct type and return values of the correct type (throwing an error if the resolved value is not of the correct type)

josephearl commented 11 months ago

Raised a similar issue on the new repo https://github.com/launchdarkly/js-core/issues/285

josephearl commented 11 months ago

This is available for testing https://github.com/launchdarkly/js-core/issues/285#issuecomment-1738239191