PostHog / posthog-ios

PostHog iOS SDK
https://posthog.com/docs/libraries/ios
MIT License
35 stars 41 forks source link

Add Callback Support to `reloadFeatureFlags` Method #76

Closed cameronehrlich closed 11 months ago

cameronehrlich commented 11 months ago

What does this PR do? This PR adds support for receiving a callback when fetching feature flags.

Where should the reviewer start? I added a new method to the PHGPostHog class that allows the user to specify a callback when fetching feature flags.

How should this be manually tested? Make sure that the user receives a callback when the feature flags are finished being fetched.

Any background context you want to provide? We would like to be able to fetch feature flags at app launch and wait to proceed until they are guaranteed to be fetched. The infrastructure appeared to be already in place, and I just hooked it up.

What are the relevant tickets? N/A

Screenshots or screencasts (if UI/UX change)

Screenshot 2023-10-18 at 1 51 27 PM

Questions:

marandaneto commented 11 months ago

@cameronehrlich thanks for the PR. My suggestion would be to not pass the callback everywhere but rather trigger a notification. We are doing this on the next major version. You could either apply the suggestion on this PR or wait for the new major if you don't need that super urgent. Take a look at the usage of didReceiveFeatureFlags.

cameronehrlich commented 11 months ago

@marandaneto great to hear that you are adding a didReceiveFeatureFlags notification in the next version!

I can definitely update this PR to post a didReceiveFeatureFlags notification, but I did want to push back very gently and with respect on the idea of not supporting a callback for this call. 🙏

From the perspective of the consumer of your SDK, while I totally agree that a having a notification could be very useful, it also is very handy to have a tight coupling between requesting the flags and knowing when they arrive, especially in a use case like ours where we want to guarantee the feature flags are loaded at launch. In fact, it looks like your team has recognized this based on the support for this that you have added in v3.0.0.

In short, I would propose that the SDK should support both a callback and a notification.

Do you have a rough estimate of when you are planning on shipping v3.0.0?

marandaneto commented 11 months ago

@marandaneto great to hear that you are adding a didReceiveFeatureFlags notification in the next version!

I can definitely update this PR to post a didReceiveFeatureFlags notification, but I did want to push back very gently and with respect on the idea of not supporting a callback for this call. 🙏

From the perspective of the consumer of your SDK, while I totally agree that a having a notification could be very useful, it also is very handy to have a tight coupling between requesting the flags and knowing when they arrive, especially in a use case like ours where we want to guarantee the feature flags are loaded at launch. In fact, it looks like your team has recognized this based on the support for this that you have added in v3.0.0.

In short, I would propose that the SDK should support both a callback and a notification.

Do you have a rough estimate of when you are planning on shipping v3.0.0?

@cameronehrlich Indeed, the new SDK has both approaches because it solves different use cases. I think the reason in this version is that methods are called dynamically so it's strange that the reload feature flags callback leaks to every single capture method that has nothing to do with it. I'd be down to merge it in though since this version is going to be deprecated soonish anyway.

Would you mind adding a changelog entry?

The first alpha of the iOS v3 should be coming within the next few days (later next week most likely), would you like to be a beta tester as soon as it is out? :)

cameronehrlich commented 11 months ago

@marandaneto I'm not entirely sure what you mean when you say it "leaks". My understanding is that the callback is allocated on the stack and essentially thrown away when it is nil with negligible impact on memory or performance. Since the new SDK is coming so soon and I don't want to mess anything up with your SDK, we can just use our fork until the new SDK drops and I'll close this PR.

marandaneto commented 11 months ago

@marandaneto I'm not entirely sure what you mean when you say it "leaks". My understanding is that the callback is allocated on the stack and essentially thrown away when it is nil with negligible impact on memory or performance. Since the new SDK is coming so soon and I don't want to mess anything up with your SDK, we can just use our fork until the new SDK drops and I'll close this PR.

@cameronehrlich Sorry for the confusion. I meant the implementation leaking, since we have to pass the callback to all the methods (capture, etc) that have nothing to do with the reloadFeatureFlags feature.