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

Add Callback Support to reloadFeatureFlags Method #76

Closed

Conversation

cameronehrlich
Copy link

@cameronehrlich cameronehrlich commented Oct 18, 2023

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:

  • Does the docs need an update?
    You may want to mention this functionality in your iOS SDK docs.
  • Are there any security concerns?
    No.
  • Do we need to update engineering / success?
    Probably not.

@cameronehrlich cameronehrlich changed the title add callback support to reloadFeatureFlags Add Callback Support to reloadFeatureFlags Method Oct 18, 2023
@cameronehrlich cameronehrlich marked this pull request as ready for review October 18, 2023 20:56
@marandaneto
Copy link
Member

@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
Copy link
Author

@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
Copy link
Member

@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
Copy link
Author

@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
Copy link
Member

@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.

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.

2 participants