search

Flagsmith / flagsmith

Open Source Feature Flagging and Remote Config Service. Host on-prem or use our hosted version at https://flagsmith.com/
https://flagsmith.com/
BSD 3-Clause "New" or "Revised" License
4.74k stars 358 forks source link

Introduce the SDK Evaluation Context object #4413

Open khvn26 opened 1 month ago

khvn26 commented 1 month ago

Abstract

While implementing #4278, a discussion has occurred about how we should approach SDK changes in the future.

The consensus is we should take after OpenFeature and implement a common evaluation context object that a single getFlags interface should accept as its only input.

I'm creating this issue as a hub for further implementation discussion and planning.

Schema proposal

You can observe and review the proposed schema in #4414.

Implementation plan

For implementation phase 1, We should settle on one of the following:

For phase 2, we should consider a single /api/v2/flags API with a schema identical to the Evaluation Context schema. Should we go forward with this, it'll allow us to fully code generate remote evaluation parts of the SDKs!

matthewelwell commented 1 month ago

My preference for the implementation is (1) as I am keen to get transient traits out there ASAP. It also gives us some time to perfect the implementation and, if we decide it's not quite right, we would only need a breaking change in a single SDK.

The only concern here would be that the documentation becomes slightly more difficult, but I think we can get around that.

matthewelwell commented 1 month ago

A few comments on the schema:

  1. I see that Traits is an object. Should this be a list?
  2. I see that environment is required, since the evaluation context (as I understand it), is provided when requesting the flags, shouldn't the environment default to what was provided when initialising the client, or are we removing that entirely from init?
  3. Let's make sure that we're consistent with our casing. Maybe I'm missing a pattern, but it's odd to me that e.g. environment, feature, etc. are lower case, but Traits is upper case.
zachaysan commented 1 month ago

I mirror @matthewelwell's comments on the approach. But other than that the overall approach seems sensible to me.

matthewelwell commented 1 month ago

A few comments on the schema:

  1. I see that Traits is an object. Should this be a list?
  2. ~I see that environment is required, since the evaluation context (as I understand it), is provided when requesting the flags, shouldn't the environment default to what was provided when initialising the client, or are we removing that entirely from init?~
  3. ~Let's make sure that we're consistent with our casing. Maybe I'm missing a pattern, but it's odd to me that e.g. environment, feature, etc. are lower case, but Traits is upper case.~

Added the one outstanding comment to the PR