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

fix: getFeatureFlagPayload wasn't sending the event #226

Closed
wants to merge 2 commits into from

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Feb 20, 2025

💡 Motivation and Context

PostHog/posthog-flutter#161 for Android
iOS would need to do the same.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@marandaneto marandaneto requested review from ioannisj and a team February 20, 2025 15:23
@marandaneto
Copy link
Member Author

before merging this, I'd like to ask if there's a reason why this is not done on the JS SDK either, since we ported the code from there.
https://github.com/PostHog/posthog-js/blob/1a5ce0bf6a7f77c44499de792ad4ec7aa45b8aa5/src/posthog-featureflags.ts#L358-L361

@rafaeelaudibert
Copy link
Member

rafaeelaudibert commented Feb 20, 2025

@marandaneto I believe it's intentional in posthog-js. You can see that we actually use getFeatureFlagPayload() inside the code that actually sends the $feature_flag_called event: https://github.com/PostHog/posthog-js/blob/1a5ce0bf6a7f77c44499de792ad4ec7aa45b8aa5/src/posthog-featureflags.ts#L346. We definitely don't want two captures in that case.

This is all very intricate in the posthog-js library, we might need to decouple that a little to figure out how this should behave. It sucks that our SDKs aren't consistent - something the sdk-team-whom-shouldnt-be-mentioned could work on.

The android SDK, for example, is sending much less information than the js SDK, even after your changes.

@ioannisj
Copy link

Another difference I see is that we don't seem to include $feature_flag_payload property in $feature_flag_called event on mobile (at least on iOS)

@marandaneto
Copy link
Member Author

$feature_flag_payload

that is new, not added to all SDKs yet

@marandaneto
Copy link
Member Author

@marandaneto I believe it's intentional in posthog-js. You can see that we actually use getFeatureFlagPayload() inside the code that actually sends the $feature_flag_called event: PostHog/posthog-js@1a5ce0b/src/posthog-featureflags.ts#L346. We definitely don't want two captures in that case.

This is all very intricate in the posthog-js library, we might need to decouple that a little to figure out how this should behave. It sucks that our SDKs aren't consistent - something the sdk-team-whom-shouldnt-be-mentioned could work on.

The android SDK, for example, is sending much less information than the js SDK, even after your changes.

Most of the new props are new https://github.com/PostHog/posthog-js/pull/1567/files
they just did not port to all SDKs yet

I think its intentional that getFeatureFlagPayload does not capture $feature_flag_called, so it's better to be clear about that first, I don't recall the reason, though.

@marandaneto marandaneto removed the request for review from a team February 21, 2025 10:21
@marandaneto
Copy link
Member Author

Closing it until @PostHog/team-feature-flags tell what's the way forward here

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