Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve feature flags #71

Merged
merged 9 commits into from
Aug 10, 2020
Merged

Improve feature flags #71

merged 9 commits into from
Aug 10, 2020

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Jul 16, 2020

Changes

No tests since there are no tests for feature flags... and I don't have time to add any now before the day is over :).

Checklist

  • Tests for new code (if applicable)
  • TypeScript definitions (module.d.ts) updated and in sync with library exports (if applicable)

@mariusandra mariusandra requested a review from timgl July 16, 2020 14:23
@timgl
Copy link
Collaborator

timgl commented Jul 16, 2020

Does it make sense to call this if identify is called? It feels like that should work automatically

@mariusandra
Copy link
Collaborator Author

We could call this directly, but we need to make sure identity is not only called, but the event is processed in the backend, giving the new feature flags. Alternatively, we could pass the new distinct_id somehow to /decide and use that to directly calculate the new flags.

@Ralph7C2
Copy link

Great! That would work if I could be sure that the call to get_distinct_id was updated, but it appears that is what you mean by "but we need to make sure identity is not only called, but the event is processed in the backend".

Would it be possible to have the onFeatureFlags handler act as an event handler? It looks like we could get around the server part by using adding a call in the persistence module, like an observer pattern. I am likely missing a lot of the big picture, and haven't tested it, but something like:

From: Ralph Landon <[email protected]>
Date: Thu, 16 Jul 2020 09:41:38 -0700
Subject: [PATCH] Add event hander for feature flag updates

---
 src/posthog-core.js        |  3 ++-
 src/posthog-persistence.js | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/posthog-core.js b/src/posthog-core.js
index b6d8385..085ebb7 100644
--- a/src/posthog-core.js
+++ b/src/posthog-core.js
@@ -910,7 +910,8 @@ PostHogLib.prototype.isFeatureEnabled = function (key) {
  * @param {Function} [callback] The callback function will be called once the feature flags are ready. It'll return a list of feature flags enabled for the user.
  */
 PostHogLib.prototype.onFeatureFlags = function (callback) {
-    return this.feature_flags.onFeatureFlags(callback)
+    return this.persistence.addFeatureFlagsHandler(callback)
+    //return this.feature_flags.onFeatureFlags(callback)
 }
 
 /**
diff --git a/src/posthog-persistence.js b/src/posthog-persistence.js
index d87a328..5a5eebe 100644
--- a/src/posthog-persistence.js
+++ b/src/posthog-persistence.js
@@ -41,6 +41,8 @@ var PostHogPersistence = function (config) {
     this['props'] = {}
     this.campaign_params_saved = false
 
+    this['featureFlagEventHandlers'] = []
+
     if (config['persistence_name']) {
         this.name = 'ph_' + config['persistence_name']
     } else {
@@ -65,6 +67,17 @@ var PostHogPersistence = function (config) {
     this.save()
 }
 
+PostHogPersistence.prototype.addFeatureFlagsHandler = function(handler) {
+    this.featureFlagEventHandlers.push(handler)
+    return true
+}
+
+PostHogPersistence.prototype.onFeatureFlags = function(flags) {
+    this.featureFlagEventHandlers.forEach(handler => {
+        handler(flags)
+    });
+}
+
 PostHogPersistence.prototype.properties = function () {
     var p = {}
     // Filter out reserved properties
@@ -196,7 +209,9 @@ PostHogPersistence.prototype.register_once = function (props, default_value, day
 PostHogPersistence.prototype.register = function (props, days) {
     if (_.isObject(props)) {
         this.expire_days = typeof days === 'undefined' ? this.default_expiry : days
-
+        if (props?.$active_feature_flags) {
+            this.onFeatureFlags(props?.$active_feature_flags)
+        }
         _.extend(this['props'], props)
 
         this.save()
-- 
2.25.1

@Ralph7C2
Copy link

Passing in the distinct_id would also work for my use case I think, basically exposing 'decide' as 'featureFlagsForDistinctID(id): flags'

@mariusandra mariusandra force-pushed the improve-feature-flags branch from 3cc557e to 309e9b8 Compare August 3, 2020 08:43
@mariusandra
Copy link
Collaborator Author

Hi @Ralph7C2! My apologies for the long wait before a reply (vacations period 😅)

I actually gave you some wrong information before. We are passing the distinct_id to the call to /decide (which responds with the feature flags), both when initializing the library and also inside reloadFeatureFlags().

If your feature flags are only dependent on the user's ID (e.g. to calculate if the user is part of the flag's rollout), what is implemented in this PR should be enough. Just call reloadFeatureFlags after identify and you're all set. Please let me know if this is enough or not..

If the flag is also calculated from some properties you pass with identify (or people.set), we're back where we started from.... and must wait for the backend queue to finish processing the event before it has an effect on the flags. I'm really not sure how to get notified of this as it's all very async.

The idea to refactor onFeatureFlags into a more event handler type patter makes sense. I'll have a look at that shortly... and try to implement @timgl 's idea of reloading the flags directly after an identify call.

@Ralph7C2
Copy link

Ralph7C2 commented Aug 7, 2020

Yes, I believe as long as calling reloadFeatureFlags directly after identify gives the feature flags for the id passed in identify.

Our flow is:

  • On navigation to landing page/sign-in/sign-up create a new random user(to start tracking immediately)
  • On login, associate the random user with the logged in user's id

All feature flagging would be after logging in, and we would just need it to be consistent on the user id. My understanding of PostHog's feature flag code is it is some hashing function applied to the feature_flag_id+distinct_id, so as long as that distinct_id is the user_id we pass to identify, it'll work?

@mariusandra mariusandra force-pushed the improve-feature-flags branch from 522ef9b to 7a4672f Compare August 10, 2020 09:44
@mariusandra
Copy link
Collaborator Author

New changes:

  • onFeatureFlags is now an event handler. You give it callbacks and they get called when feature flags change.
  • reloadFeatureFlags is now called on identify and triggers the callbacks that were sent to onFeatureFlags

Problems:

@timgl ready to be reviewed

@mariusandra
Copy link
Collaborator Author

Merged with master and one change added:

  • To avoid breaking existing behaviour, the onFeatureFlags(callback) function calls the callback immediately if the flags are loaded. Plus it gets called again when the flags change.

Ready for a look.

@timgl
Copy link
Collaborator

timgl commented Aug 10, 2020

on it

@timgl timgl merged commit 8300081 into master Aug 10, 2020
@timgl timgl deleted the improve-feature-flags branch August 10, 2020 12:54
@mariusandra
Copy link
Collaborator Author

@Ralph7C2 this is in v1.4.1 (changelog). Please report back if it works for you!

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.

Feature flag issue for SPA Feature flag gives incorrect warning
5 participants