feat: send client info to Flagsmith

[tiagoapolo]

Ref: #2479

• This pull request includes changes to the flagsmith-core.ts file to add support for appName and appVersion in the Flagsmith class.
• Updated the getJSON method to include X-Customer-Application-Name and X-Customer-Application-Version headers if appName and appVersion are provided.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

PR Overview

This pull request adds support for sending client application details (appName and appVersion) to Flagsmith through HTTP headers.

• Updated Config type to include appName and appVersion fields.
• Added corresponding properties and initializations to the Flagsmith class.
• Updated getJSON to add the new headers based on the provided client info.

Reviewed Changes

File	Description
flagsmith-core.ts	Adds support for client info through headers and config updates

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[khvn26]

Do we want to make the new properties a part of the shared evaluation context interface?

CC @matthewelwell @kyle-ssg

[Copilot]

PR Overview

This PR adds support for sending client information (appName and appVersion) to Flagsmith via HTTP headers, enhancing configuration flexibility when initializing the Flagsmith client.

• Extended the Config type and Flagsmith class to support appName and appVersion.
• Updated the getJSON method to send X-Customer-Application-Name and X-Customer-Application-Version headers.

Reviewed Changes

File	Description
flagsmith-core.ts	Updated configuration and HTTP request handling to include appName and appVersion

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[matthewelwell]

Do we want to make the new properties a part of the shared evaluation context interface?

CC @matthewelwell @kyle-ssg

@khvn26 I'm not sure if these properties should be part of the evaluation context? Would we expect them to change?

[khvn26]

@matthewelwell We wouldn't during client runtime, but actually segmenting by app name and app version in evaluation could be desirable for the future?

[tiagoapolo]

@matthewelwell We wouldn't during client runtime, but actually segmenting by app name and app version in evaluation could be desirable for the future?

That makes sense, we could use it the future

[matthewelwell]

Ok, sounds good.

@khvn26 @tiagoapolo do you guys have an idea of the changes that are needed then?

[khvn26]

I've opened a PR to update the schema here: Flagsmith/flagsmith#5203

After it's merged, we'll be able to regenerate evaluation-context.ts

[rolodato]

(ignore)

[Copilot]

Pull Request Overview

This pull request adds support for sending client information (app name and version) to Flagsmith via custom HTTP headers. Key changes include:

• Updating the Flagsmith configuration to accept appName and appVersion.
• Modifying the getJSON method to include the "X-Customer-Application-Name" and "X-Customer-Application-Version" headers when these values are provided.
• Adding tests to validate that the headers are set appropriately based on provided configuration.

Reviewed Changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

File	Description
test/init.test.ts	Added tests to verify header inclusion and exclusion based on config; note that duplicate tests exist with the same description.
flagsmith-core.ts	Extended configuration and getJSON functionality to support appName and appVersion headers.

Files not reviewed (2)

• lib/flagsmith/package.json: Language not supported
• lib/react-native-flagsmith/package.json: Language not supported

Comments suppressed due to low confidence (1)

test/init.test.ts:296

• Duplicate test case detected with the same description as an earlier test (line 275). Consider merging or renaming one of the tests to ensure each test case verifies a unique scenario and to reduce potential maintenance overhead.

test('should send app name and version headers when provided', async () => {

[rolodato]

The code itself looks fine, but I'm curious why we chose this approach instead of setting these attributes as traits. I can speculate as to why:

• We don't have reserved trait names, so we can't guarantee there will not be collisions with existing customer traits. This is still a solvable problem, with some effort
• Customers that track application attributes would always have to use the /identities endpoint (probably with transient traits), since /flags does not accept traits

My impression is that setting these attributes as traits is the better option; it has its challenges, but I don't see what we would gain by tracking them separate from traits. By using traits, every SDK (including OpenFeature) automatically supports this and we don't have to adapt clients in any way.

Another question of asking this would be - if customers want to target flags based on application name/version/platform/etc, they can do that right now with traits. What would they gain by using special headers instead? I'm sure I'm missing some context to be able to answer this question.

[tiagoapolo]

The code itself looks fine, but I'm curious why we chose this approach instead of setting these attributes as traits. I can speculate as to why:

• We don't have reserved trait names, so we can't guarantee there will not be collisions with existing customer traits. This is still a solvable problem, with some effort
• Customers that track application attributes would always have to use the /identities endpoint (probably with transient traits), since /flags does not accept traits

My impression is that setting these attributes as traits is the better option; it has its challenges, but I don't see what we would gain by tracking them separate from traits. By using traits, every SDK (including OpenFeature) automatically supports this and we don't have to adapt clients in any way.

Another question of asking this would be - if customers want to target flags based on application name/version/platform/etc, they can do that right now with traits. What would they gain by using special headers instead? I'm sure I'm missing some context to be able to answer this question.

The goal is to track usage and using headers for that case is more appropriate. Also, we don't want to store that information in traits for future evaluation for now, that can come up in a new task/PR.

cc: @matthewelwell

[rolodato]

The code LGTM, but I'd like to understand more about how customers are expected to use this feature. It's difficult to judge this PR in isolation without understanding the full scope of this feature.

I also wonder if we should add a distinct identifier, separate from the application name. This would let customers rename their application "friendly name", and not be stuck forever with an outdated name.

For example, imagine if we set applicationMetadata.name: "Bullet Train", and we later want to rename this to Flagsmith. We would have to choose to either be stuck forever with this name, or lose continuity with any data collected until that point.

[Zaimwa9]

@rolodato I get your point. It would be critical when be used for traits to avoid any discontinuity in case of value change.

(I'm repeating also to confirm my understanding of the objective) In the context of gathering usage and tracking data, as long as they are informational it wouldn't bring any disruption of service to change the name of an application.
Because we have the API key on which we could rely to track [project,environment], I see this as a way to group together requests/usage of a sub-application on a given environment, answering the question:

• For my production environment, with 4 different applications using the same key, how is my usage distributed.

So with a name change, it would cause a spike change (from bullet to flagsmith) but the actionability of the underlying data would stay the same imo ("we need to decrease our usage in this one").

I agree with you though, as soon as this is used for traits and evaluation it will need to have a persistent key over time even with name changing etc

In the context of this PR I personally think it's not blocking and it's a base for future finetuning

[Zaimwa9]

LGTM codewise and working as a charm on my side 👍 . Would just need to update the doc accordingly. And to monitor whether values should be validated (max nb of characters, trimming etc)
Approving if no behavioral changes related to the discussion.