feat: support type-safe variant values for InMemoryProvider#1356
feat: support type-safe variant values for InMemoryProvider#1356MattIPv4 wants to merge 4 commits intoopen-feature:mainfrom
Conversation
Summary of ChangesHello @MattIPv4, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the type safety of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
aa6c00f to
e2caf7d
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces generics to the InMemoryProvider and its associated flag configuration types to enable type-safe variant values. This is a significant improvement, ensuring that defaultVariant and the return values from contextEvaluator are valid keys within the variants object. The implementation leverages advanced TypeScript features effectively, and the accompanying test updates are thorough, correctly using as const and @ts-expect-error to validate the new type safety. Overall, this is a well-executed feature enhancement.
packages/web/src/provider/in-memory-provider/in-memory-provider.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
Signed-off-by: MattIPv4 <me@mattcowley.co.uk>
0e7d325 to
0b723eb
Compare
beeme1mr
left a comment
There was a problem hiding this comment.
Hey @MattIPv4, thanks for the PR. It's nice a great improvement to the in memory provider. However, it appears to be a slight breaking change. Is this expected?
Type error
const booleanFlagSpec = {
'a-boolean-flag': {
variants: {
on: true,
off: false,
},
disabled: false,
defaultVariant: 'on',
},
}
await provider.putConfiguration(booleanFlagSpec);
^ Type 'string' is not assignable to type '"on" | "off"'.ts(2345)No type error
Type error
const booleanFlagSpec = {
'a-boolean-flag': {
variants: {
on: true,
off: false,
},
disabled: false,
defaultVariant: 'on',
},
} as const;
await provider.putConfiguration(booleanFlagSpec);I understand why the const is important to the types but it would be nice if it fell back to the non-type safe version if the definition isn't a readonly type.
|
Yeah, as I said in the notes, that is expected as a "breaking" change, though my view would be that it isn't actually a breaking change to the API that the SDK offers, it is simple enough for folks to add I'm not sure I'm keen on the idea of having it fall back to not being type safe if it cannot infer correctly, that feels like it'd end up being a DX trap for folks thinking it is acting in a type-safe way when actually it isn't because it silently failed to infer the types it needed? |
This PR
Applies a few generics to the
FlagandFlagConfigurationtypes used byInMemoryProviderto allow for type-safe variant values, ensuring thedefaultVariantand any value returned bycontextEvaluatorare present as keys invariantsfor the given flag.Related Issues
Resolves #967
Closes #1046
Notes
If folks are passing in an object reference to
InMemoryProvider, like we are in the specs, rather than an object literal directly, this is technically a breaking change as they'll need toas constor pass it directly, to allow TypeScript to correctly infer the types. I'm personally of the opinion that this isn't a breaking change in the API of the package, and so this can ship as a minor change, but open to being told this should be considered breaking.Also, the monorepo is using a very old version of TypeScript, 4.4, so I've had to include a util for
NoInferrather than using the built-in util that's available in TypeScript 5.4 and beyond.Follow-up Tasks
N/A
How to test
See specs where this is already demonstrated to be working as I've had to apply
@ts-expect-errorwhen we're using invalid variant keys.