Skip to content

Conversation

@kyle-ssg
Copy link
Member

@kyle-ssg kyle-ssg commented Feb 6, 2025

This will allow users to generate a typescript file of all flags belonging to a project and all the possible types for remote configuration based on the environment values

This will allow

  • Typed autocomplete of any existing features as well as possible remote configuration values
  • Detection of incorrectly named features / features that no longer exist
  • "Dry runs" of removing a feature with compilation checking

Type generation

export FLAGSMITH_API_KEY=API_KEY flagsmith-generate-types PROJECT_ID

The cli exports a file that looks like the following

export interface FlagsmithTypes {
    "4eyes":                              null;
    a_temp_feature:                       number;
    allow_client_traits:                  null;
    announcement:                         Announcement;
}
export type export interface Announcement {
    id?:          string;
    title?:       string;
    description?: string;
    buttonText?:  string;
    url?:         string;
    isClosable?:  boolean;
}

Using the types file

The flagsmith JS SDK will be able to use the generated types file from version 9.0.3, see See Flagsmith/flagsmith-js-client#298

Typechecking the removal of a flag supersedes code reference searches

Due to this typesafety, this means we can test the removal of a flag. The CLI provides a way of generating types excluding a CSV list of flags.

export FLAGSMITH_API_KEY=API_KEY flagsmith-generate-types PROJECT_ID -e announcement,other_flag

If an application is setup to use these generics any missed code references will be caught at compile time e.g.

1 - Adding a remote config.mov

1.-.Adding.a.remote.config.mov

2 - Creating a new flag.mov

2.-.Creating.a.new.flag.mov

3 - Dry run removing flag.mov

image

3.-.Dry.run.removing.flag.mov

@kyle-ssg kyle-ssg marked this pull request as draft February 6, 2025 13:28
…enerate-types

# Conflicts:
#	src/commands/generate-types/index.ts
#	src/util/get-all-features.ts
@kyle-ssg kyle-ssg changed the title feat: generate types feat: Add interface types Mar 19, 2025
@kyle-ssg kyle-ssg requested a review from tiagoapolo March 19, 2025 18:20
@kyle-ssg kyle-ssg marked this pull request as ready for review March 25, 2025 11:13
@kyle-ssg kyle-ssg requested a review from rolodato March 25, 2025 11:13
const output = flags.output

await jsonToTypescript(jsonString).then(ts => {
console.log('Outputting types to', output)
Copy link
Contributor

Choose a reason for hiding this comment

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

All log output should be to stderr and not stdout

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm not sure what you mean really, showing where the file is being outputted is useful and not an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is only so the output can be used as the input for other scripts, and the output is not polluted by log output. It doesn't mean that this or any other log lines are errors, just that they should be outputted to stderr which is a general best practice for logging.

return fetch(`${api}/api/v1/${url}`, {
headers: {
'Content-Type': 'application/json',
AUTHORIZATION: isMasterAPIKey(apiKey) ? `Authorization Api-Key ${apiKey}` : `Token ${apiKey}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? We're setting AUTHORIZATION: Authorization Api-Key ${apiKey} in the first case

Copy link
Contributor

Choose a reason for hiding this comment

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

Also IMO we should not be inspecting the content of API keys. This is a bug that the API needs to fix Flagsmith/flagsmith#5146

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's correct. And yeah agree the header should work for both, but I'm unsure when this will get resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that the header's value is being set to Authorization Api-Key ${apiKey} in one case, but just Token ${apiKey} in the other. The header's value in the first case has an extra Authorization which I had never seen before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh weird, what's weirder is that it works haha - will remove

const {api, apiKey, project} = data
const getReq = doGet(api, apiKey)
const environments: { api_key: string, id: number }[] = await getReq(`projects/${project}/environments/`)
return Promise.all(environments.map(v => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to iterate through all environments to get the feature values? How do we deal with conflicting data types between environments?

Copy link
Member Author

@kyle-ssg kyle-ssg Apr 9, 2025

Choose a reason for hiding this comment

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

This is basically the whole point of this, it figures out all possible types and creates a union / optional types where necessary. This actually helps capture people having incorrect configs across environments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to elaborate on this, here's a couple examples

// It recognises that some fields are optional
export interface SegmentOperator {
    value:             string;
    label:             string;
    type?:             Type;
    append?:           Append;
    valuePlaceholder?: ValuePlaceholder;
    warning?:          string;
    hideValue?:        boolean;
}

announcement:                         Announcement;
// it recognises some environments don't use this 
announcement_per_page:                Announcement | null;

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but I'm not sure I understand the expected behaviour. If I have conflicting types in multiple environments, is the expected behaviour to generate a union type that captures them if possible, or to throw an error because we can't determine what the correct type should be? Should all environments always be considered to determine the correct type, or should users be able to choose which ones to look at? What about adding new flags, is the expectation to create them in Flagsmith first and then generate the types for them, or the other way around? How do you do that without affecting the types that have already been generated for other environments?

I see many assumptions on how this should work that we've made, and I'm not sure if we've fully thought out the workflows that customers will want to use for this.

Copy link
Member Author

@kyle-ssg kyle-ssg Apr 15, 2025

Choose a reason for hiding this comment

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

It generates unions for conflicting types, the purpose of this is for the types in code to reflect what is actually set.

I suppose it could consider a specific individual/list of environments, but ultimately that code would then be unsafe if it pointed to an environment that wasn't considered and has the wrong types just the same as if the a JSON schema was adjusted.

Adding new flags is as per the video example.

  • Create the flag, re-run the types.

To be honest I've considered all of these because I'd quite like this functionality for several projects.

for (const key of Object.keys(instance.getAllFlags())) {
let value = instance.getValue(key)
try {
value = JSON.parse(instance.getValue(key))
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we don't have an explicit JSON data type in Flagsmith, I'm not sure it's a good idea to try and parse JSON out of strings. We don't know anything about the expected data type of each property, what's optional and what's not, etc.

I wonder if it's better to return only the data types Flagsmith currently supports, and let customers implement their own typechecking for anything beyond that.

Copy link
Member Author

@kyle-ssg kyle-ssg Apr 9, 2025

Choose a reason for hiding this comment

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

Not sure I agree, most of the value is in defining these JSON types it requires the most effort to maintain and is also the most likely to catch people's mistakes. The videos demonstrate these use cases. I actually don't think there's much value at all in defining types for just primitives.

This figures out what's optional and what's not based on the actual data.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that JSON is only one type of format that customers could use to store values. If we're sticking with only what our frontend supports, this could also be plain text, XML, TOML or YAML.

This figures out what's optional and what's not based on the actual data.

I don't understand how this could work by only looking at the data. For example, if a flag's value has a certain JSON key set in all environments, does that actually mean that the key is required, or could it be optional in new environments? If it should be optional, are customers expected to manually edit the types generated by this command and save those types in their source control?

I get the impression that we might be solving this problem backwards, by deducing the customer's types rather than being explicitly told by the customer what those should be. I see how can this would be useful to bootstrap types based on existing feature values, but I'm not sure what the full workflow looks like beyond that, and how to prevent configuration drift between the local application types and the actual Flagsmith values.

Copy link
Member Author

@kyle-ssg kyle-ssg Apr 15, 2025

Choose a reason for hiding this comment

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

For example, if a flag's value has a certain JSON key set in all environments, does that actually mean that the key is required

No, the types reflect what is actually set. If a key is not set in certain environments then it's optional since some don't have it. This protects against a developer assuming something is set just because they see it in 1 environment.

I'm not sure what the full workflow looks like beyond that, and how to prevent configuration drift between the local application types and the actual Flagsmith values.

If there's a drift between local and remote values that's breaking then there's probably code that needs to be fixed assuming a field got removed, that would essentially make a field that was once guaranteed now optional. It's useful for this to be highlighted as soon as it can be. I suppose if it was relied upon in CI you could specify the environment it was being built to, but local development would often want to consider all of them.

I get the impression that we might be solving this problem backwards, by deducing the customer's types rather than being explicitly told by the customer what those should be.

I think the 2 approaches would naturally work together, forcing a JSON Schema against the feature would just result in the types generated through this to be consistent since they wouldn't be able to save anything incorrect.

In a perfect world, a schema would be supplied against JSON remote config, however that's a very large amount of manual upkeep and would put a lot of people off. This provides the quality of life of getting the types without having to go through the extra effort of adjusting a schema every time remote config is adjusted.

Copy link
Member Author

@kyle-ssg kyle-ssg Apr 15, 2025

Choose a reason for hiding this comment

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

My point is that JSON is only one type of format that customers could use to store values. If we're sticking with only what our frontend supports, this could also be plain text, XML, TOML or YAML.

It'll treat XML|TOML|YAML as string, this doesnt attempt to provide any typings for them. "Text" is actually number, string or boolean - this generates whatever type it is

const {lines} = await quicktype({
inputData,
lang: 'typescript',
rendererOptions: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Quicktype supports many languages and options, and I wonder if everyone would be happy with these defaults. By depending on Quicktype we're also now responsible for keeping this version up to date or dealing with backwards compatibility, since we can't use provided dependencies in a CLI tool.

Instead of generating types directly, have we considered outputting quicktype-compatible JSON instead? This output could be piped into npx quicktype -l typescript and give the same result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative could be to implement an API endpoint that exposes an QuickType-compatible definition for a project, and let customers consume that however they find convenient.

Copy link
Member Author

@kyle-ssg kyle-ssg Apr 9, 2025

Choose a reason for hiding this comment

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

I think it'd be very interesting to support languages other than Typescript in the future. Maybe we could allow overriding some of the quicktype config, I wonder if this could eventually live in a flagsmithconfig.json rather than passing a load of arguments to the cli.

The primary goal of this is ease of use and convenience, I think it's a good thing to hide the complexities of quicktypes given that this configuration results in a well defined set of types. If we piped converting the json to x I feel like we'd want to remove the json file at the end since it'd just be clutter.

Copy link
Contributor

Choose a reason for hiding this comment

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

We wouldn't even need to save the JSON file - it could be output to stdout so that it can be directly piped into quicktype. Being able to support all quicktype languages seems like a good enough reason to not assume the default output should be TypeScript IMO, and to avoid coupling our CLI config with a bunch of quicktype config.

Copy link
Member

@khvn26 khvn26 Apr 14, 2025

Choose a reason for hiding this comment

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

Another alternative could be to implement an API endpoint that exposes an QuickType-compatible definition for a project, and let customers consume that however they find
convenient.

Ideally, this "QuickType-compatible" definition should be a JSON schema so it's usable with other code generators (e.g. like datamodel-codegen) as well. This would tie in nicely with a JSON Schema validation feature we're going to eventually implement.

Another another alternative to think about is exposing an endpoint that generates an OpenFeature CLI flag manifest, used by the CLI to generate typed OpenFeature clients. They don't currently support specific types for structured values, so we could either try and move the quicktype work over to their CLI (and help standardise user JSON schemas for feature values), or simply offer ours as an alternative for users seeking advanced feature value typing.

@kyle-ssg kyle-ssg requested a review from rolodato April 9, 2025 10:52
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.

5 participants