-
Notifications
You must be signed in to change notification settings - Fork 127
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: Support for client-assigned feature flags #1519
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size Change: +20.3 kB (+0.66%) Total Size: 3.12 MB
ℹ️ View Unchanged
|
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 is interesting work; what's the context here? Is this supporting some sort of customer request?
return Number(hashInt) / Number(LONG_SCALE) // Normalize the hash to a value between 0 and 1 | ||
} | ||
|
||
// TODO how much do we trust sonnet to write a hashing function? |
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.
😆
} | ||
|
||
// TODO how much do we trust sonnet to write a hashing function? | ||
_hash(input: string): string { |
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.
we use sha1
in both the python and rust implementations when creating hash keys; any reason why we wouldn't do the same 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.
any reason why we wouldn't do the same here?
@dmarticus Tell me more? I ran into some IE11 linting issue with crypto
, but maybe we can actually use it.
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.
argh, should've known there's some browser compat issue – there's some prior art (slack, github around separating our bundles into es5 (for things that explicitly need IE11 support) and es6 (for everything else), but I'm not sure what our policy on adding new behavior that explicitly doesn't support IE11 is.
It might be a pain to support it here though, even if crypto
is technically "supported" on IE11, it seems like the support pegged to a specific version of that library, and I'm not sure if taking on the overhead of making sure we have IE11 support is worth using the library. Then again, using a standard for crypto is probably safer than rolling our owning hashing algo. TBH – the decision is up to you, I just wanted to provide my 2 cents. I definitely didn't realize this was a browser compat thing beforehand.
@dmarticus Yep, see https://posthog.slack.com/archives/C07PXH2GTGV/p1731099028903259 |
great, thanks for the context! |
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Closing in favor of https://posthog-fast-feature-flags.vercel.app/ for now If it turns out to work well, I'll submit a new PR |
Changes
Adds support for client-assigned feature flags. When
bootstrap.clientAssignedFeatureFlags
is provided, the specified feature flags will be assigned with values based on the PostHog backend's allocation strategy.Related PostHog/posthog#26145
Checklist