- 
                Notifications
    You must be signed in to change notification settings 
- Fork 71
Feature/custom types #771
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
base: main
Are you sure you want to change the base?
Feature/custom types #771
Conversation
        
          
                src/client.ts
              
                Outdated
          
        
      | */ | ||
| export interface CustomTypeOptions {} | ||
|  | ||
| type GetCustom<K extends PropertyKey, Fallback> = | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would type GetCustom<K extends PropertyKey, Fallback> = K extends keyof CustomTypeOptions ? CustomTypeOptions[K] : Fallback; achieve the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should be the same. Updated.
| Pull Request Test Coverage Report for Build 18814047319Details
 
 💛 - Coveralls | 
| import { initialize } from 'unleash-client'; | ||
|  | ||
| declare module 'unleash-client' { | ||
| interface CustomTypeOptions { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to make type safe variant names or add extra type safety to other parts of the feature flag domain this interface name may be limiting. My understanding is that this name is inspired by some other libraries doing interface merging for user enhanced type safety. What are the pros and cons of calling the interface FeatureFlagNames or something more specific and intention revealing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inspired by i18next's user custom type. https://www.i18next.com/overview/typescript#create-a-declaration-file. I took the same interface name so we could add any other custom user types to the interface in the future. But actually I don't have strong opinions on it. So I will leave it to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about? UnleashTypes { flagNames: 'aaa' | 'bbb'} It will make it obvious from the interface name what you're changing and the property will be explicit that we're affecting flag names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a great change. Thanks for your contribution. Before we merge it I'd like to decide on the exported interface name since it will be part of the public contract.
| If I was looking at this type declaration, I'm not sure I'd understand what we wanted to use CustomTypeOptions for. Though this reminds of what we have in the rust-sdk, personally I'd prefer if the type was ValidFeatureNames or something similar. | 
About the changes
This change ensures that only specific feature names can be used with
isEnabled, catching typos at compile time.