Flagsmith / flagsmith-js-client

Javascript Client for Flagsmith. Ship features with confidence using feature flags and remote config. Host yourself or use our hosted version at https://www.flagsmith.com/
https://www.flagsmith.com/
BSD 3-Clause "New" or "Revised" License
59 stars 36 forks source link

Support fallback for hasFeature, add options.skipAnalytics #244

Closed rolodato closed 1 week ago

rolodato commented 2 weeks ago

This change solves https://github.com/Flagsmith/flagsmith-js-client/issues/243 by adding a fallback option to hasFeature, instead of changing how defaultFlags works. It also deprecates the current way of using skipAnalytics but preserves backwards compatibility.

I chose to move skipAnalytics into an option property instead of a boolean parameter for both getValue and hasFeature. If we chose to only add parameters to hasFeature and not deprecate anything, these would be our options:

  1. Add a { fallback: boolean } parameter
// forces you to specify a value for skipAnalytics, which is weird
hasFeature("my_bool", false, { fallback: true })

// looks inconsistent with getValue
getValue("my_feat", { fallback: "foo" }, true)
  1. Add a boolean parameter for the fallback
// easy to confuse which is skipAnalytics and which is the fallback
hasFeature("my_bool", false, true)

// easy to confuse with OpenFeature's getBooleanValue
// https://openfeature.dev/docs/reference/concepts/evaluation-api/#basic-evaluation
getBooleanValue("my_bool", true)

// still inconsistent with getValue
getValue("my_feat", { fallback: "foo" })

Putting everything under an option object also lets us add more options if needed without worrying about backwards compatibility or parameter placement/ergonomics.

The new way of using these methods would be:

hasFeature("my_bool", { fallback: true, skipAnalytics: true })
getValue("my_feat", { fallback: "foo", skipAnalytics: true })

Missing from this PR:

kyle-ssg commented 1 week ago

hey @rolodato approach looks fine, added tests and bumped version - let's go ahead with this!

rolodato commented 1 week ago

Nice, thanks! This should be a minor version bump though, not patch

kyle-ssg commented 1 week ago

Ah yep sure.