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

chore(flags): use the new /flags endpoint to manage flag evaluation for team 2 #1841

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dmarticus
Copy link
Contributor

Changes

Basically, what I'm doing is that any request with the PostHog team 2 public API key, we use /flags, and then for everything else we just use /decide.

There's a little bit of trickiness w.r.t remote config (I added a comment to that end) since new flags relies on the fact that remote config takes care of all the other non-flag-related code (it basically does away with /decide altogether). But if a customer isn't using remote config, they'll need to hit /decide anyway. I should have this handled in the logic, but worth calling out.

Copy link

vercel bot commented Mar 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Mar 21, 2025 10:02pm

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR introduces a new /flags endpoint specifically for PostHog team 2's feature flag evaluation, while maintaining the existing /decide endpoint for other customers.

  • Added DEFAULT_POSTHOG_APP_API_KEY constant to identify PostHog team 2 requests in /src/constants.ts
  • Modified feature flag evaluation in /src/posthog-featureflags.ts to use /flags?v=2 endpoint when token matches team 2's API key and remote config is enabled
  • Added logic to handle remote config interaction, ensuring /decide endpoint is still called when needed for non-flag functionality
  • Added eligibility check eligibleForFlagsV2 to determine when to use the new endpoint

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

github-actions bot commented Mar 20, 2025

Size Change: +1.31 kB (+0.04%)

Total Size: 3.63 MB

Filename Size Change
dist/array.full.es5.js 301 kB +131 B (+0.04%)
dist/array.full.js 381 kB +131 B (+0.03%)
dist/array.full.no-external.js 380 kB +131 B (+0.03%)
dist/array.js 189 kB +131 B (+0.07%)
dist/array.no-external.js 187 kB +131 B (+0.07%)
dist/main.js 189 kB +131 B (+0.07%)
dist/module.full.js 381 kB +131 B (+0.03%)
dist/module.full.no-external.js 380 kB +131 B (+0.03%)
dist/module.js 189 kB +131 B (+0.07%)
dist/module.no-external.js 187 kB +131 B (+0.07%)
ℹ️ View Unchanged
Filename Size
dist/all-external-dependencies.js 220 kB
dist/customizations.full.js 14.1 kB
dist/dead-clicks-autocapture.js 14.5 kB
dist/exception-autocapture.js 9.94 kB
dist/external-scripts-loader.js 2.75 kB
dist/posthog-recorder.js 211 kB
dist/recorder-v2.js 115 kB
dist/recorder.js 115 kB
dist/surveys-preview.js 71.3 kB
dist/surveys.js 76.6 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

Copy link
Contributor

@haacked haacked left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@@ -390,10 +391,12 @@ export class PostHogFeatureFlags {
data.disable_flags = true
}

const eligibleForFlagsV2 = token === DEFAULT_POSTHOG_APP_API_KEY && this.instance.config.__preview_remote_config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we could just do

Suggested change
const eligibleForFlagsV2 = token === DEFAULT_POSTHOG_APP_API_KEY && this.instance.config.__preview_remote_config
const eligibleForFlagsV2 = this.instance.config.__preview_flags_v2

so that I can run this on localhost easily?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, and then we can just set the preview to true for our own posthog JS initialization + any other instances of it.

That's a good idea; I might still want to use API tokens down the road (especially when rolling out to specific users), but I'm happy doing something like this instead. Thanks for the suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robbie-c went ahead and went with the config-based approach, although I might need to use API tokens down the road too (so I can turn this on for specific customers as needed). I think my plan will be to have a set of tokens that have this feature toggled on, and then allow folks to specify it in their config as well.

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.

3 participants