-
Notifications
You must be signed in to change notification settings - Fork 140
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
feat: deprecate featureFlags.override
in favor of featureFlags.overrideFeatureFlags
, a new function that supports overriding flags and flag payloads
#1697
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/posthog-featureflags.ts
Outdated
const overriddenPayloads = this.instance.get_property(PERSISTENCE_OVERRIDE_FEATURE_FLAG_PAYLOADS) | ||
|
||
if (!overriddenPayloads) { | ||
return flagPayloads || {} | ||
} | ||
|
||
const finalPayloads = extend({}, flagPayloads || {}) | ||
const overriddenKeys = Object.keys(overriddenPayloads) | ||
for (let i = 0; i < overriddenKeys.length; i++) { | ||
finalPayloads[overriddenKeys[i]] = overriddenPayloads[overriddenKeys[i]] | ||
} | ||
return finalPayloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same pattern that we do in getFlags
src/posthog-featureflags.ts
Outdated
if (triggerFlagEvent) { | ||
this._fireFeatureFlagsCallbacks() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be sufficient to trigger hooks like useFeatureFlagEnabled
, which is what I was yapping about here: https://posthog.slack.com/archives/C07Q2U4BH4L/p1738088710814869
Size Change: +10.2 kB (+0.31%) Total Size: 3.29 MB
ℹ️ View Unchanged
|
src/posthog-featureflags.ts
Outdated
* @param {boolean} [triggerFlagEvent] Optional parameter to trigger the _fireFeatureFlagsCallbacks() event. | ||
* If set to true, calling `override` will trigger the callback, which is useful for triggering things like the `useFeatureFlagEnabled` hook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I make this default true
? It's definitely new behavior, but perhaps users want it? Just saw this thread on this type of thing and it seems like the user is expecting that override
methods work like bootstrapping methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so and would even go as far as to say we might not even want to add an argument for this? Even our docs seem to communicate overrides as being the same as bootstraps outside of how long the value is persisted: https://posthog.com/docs/feature-flags/bootstrapping#overriding-feature-flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement and test coverage!
override
to support feature flag payload overrides and give users to ability to optionally trigger feature flag callback events from override
featureFlags.override
in favor of featureFlags.overrideFeatureFlags
* Override feature flags on the client-side. Useful for setting non-persistent feature flags, or for testing/debugging | ||
* feature flags in the PostHog app. | ||
/** | ||
* @deprecated Use overrideFeatureFlags instead. This will be removed in a future version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @deprecated Use overrideFeatureFlags instead. This will be removed in a future version. | ||
*/ | ||
override(flags: boolean | string[] | Record<string, string | boolean>, suppressWarning: boolean = false): void { | ||
logger.warn('override is deprecated. Please use overrideFeatureFlags instead.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging a deprecation warning too; is this overkill? cc @havenbarnes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me, good to have the forcing function so no one gets completely blindsided later when this gets removed
src/posthog-featureflags.ts
Outdated
*/ | ||
override(flags: boolean | string[] | Record<string, string | boolean>, suppressWarning: boolean = false): void { | ||
overrideFeatureFlags( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implement the new method here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if we're here changing the shape of the method signature anyway, should we go ahead and transition to a single object parameter (I'd suggest making flags
optional too) to keep things nice and clean?
Reason I bring this up is that if someone is only interested in overriding payloads, they will always have to pass some empty value in for flags
which is a tad annoying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handled this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left one nit on the new method's arguments. Also we'll wanna update the code example here https://posthog.com/docs/feature-flags/bootstrapping#overriding-feature-flags
featureFlags.override
in favor of featureFlags.overrideFeatureFlags
featureFlags.override
in favor of featureFlags.overrideFeatureFlags
, a new function that supports both override flags and overriding flag payloads
featureFlags.override
in favor of featureFlags.overrideFeatureFlags
, a new function that supports both override flags and overriding flag payloadsfeatureFlags.override
in favor of featureFlags.overrideFeatureFlags
, a new function that supports overriding flags and flag payloads
yup, doing that here: PostHog/posthog.com#10528 |
Changes
The context for these changes are this feature request (#1304) and this slack thread: https://posthog.slack.com/archives/C07Q2U4BH4L/p1738088710814869. TL;DR – if folks want to use
override
to manage the value of the feature flag in the client side, I don't see any reason not to.To make this change ergonomically (I was frustrated by the semantics of
override
, since to override payloads I'd have to do something likeoverride($flagValue, $some_bool, $payload)
, instead of just letting users dooverride($flagValue, $payload)
without needing to specify the override warning), I ended up deprecatingoverride
and replaced it with the more ergonomicoverrideFeatureFlags
. Even thoughoverride
is deprecated, though, it still works – it just calls that new method under the hood. I added the@deprecated
flag and a warning so that folks on the latest version of posthog-js move away from it.Closes #1304
Updated the docs around this change to support using the new method, too: https://github.com/PostHog/posthog.com/pull/10528/files
Checklist