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

Dynamically initialize posthog #113

Closed
wants to merge 15 commits into from
Closed

Conversation

Mastersam07
Copy link

💡 Motivation and Context

Posthog initialization can only be done natively from android manifest and info.plist. However, there are cases where user want to configure dynamically from the dart side.

A good example is loading the posthog api key and host from some 3rd party(say api or firebase remote config) and then setting up posthog.

The current setup does not allow

This PR aims at solving the above described issue.

💚 How did you test it?

With the example code, on an emulator.

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

@Mastersam07 Mastersam07 requested a review from marandaneto as a code owner July 31, 2024 07:47
@Mastersam07 Mastersam07 marked this pull request as draft July 31, 2024 07:47
@Mastersam07 Mastersam07 marked this pull request as ready for review July 31, 2024 07:49
@marandaneto
Copy link
Member

@Mastersam07 sorry I am out on sick leave and will come back to it either later this week or next week.

@GowthamanRavichandran3
Copy link

Hi @marandaneto, This PR will also fulfill our requirements. So, please merge this PR as soon as possible.

Thanks,
Gowtham.

@@ -56,86 +48,56 @@ class PosthogFlutterPlugin : FlutterPlugin, MethodCallHandler {
}
PostHogAndroid.setup(applicationContext, config)

} catch (e: Throwable) {
Copy link
Member

Choose a reason for hiding this comment

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

This should not change, Throwable is lower level

@@ -15,37 +15,29 @@ import io.flutter.plugin.common.MethodChannel.Result

/** PosthogFlutterPlugin */
class PosthogFlutterPlugin : FlutterPlugin, MethodCallHandler {
/// The MethodChannel that will the communication between Flutter and native Android
Copy link
Member

Choose a reason for hiding this comment

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

Some comments were removed, please add them back, those comments are good for new comers

print("[PostHog] com.posthog.posthog.API_KEY is missing!")
return
}
print("\nApiKey:", apiKey)
Copy link
Member

Choose a reason for hiding this comment

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

prints should be removed

@marandaneto
Copy link
Member

@shuttlershq I think it's ok to offer an alternative to use the given configuration, but the current form should still be possible to keep back compatibility.
it'd help solve this issue as well #22
my suggestion is that we offer a way to disable the auto init via native (manifest/plist), and then someone can call config manually, otherwise, everything should work as it is, wdyt?

for example:
<meta-data android:name="com.posthog.posthog.AUTO_INIT" android:value="false" /> // defaults to true
in this case, the call to posthog.config(...) is needed, otherwise, everything works as it is.

@dharanidharandharmasivam

@marandaneto , can you please provide some approximate timeline to merge this changes to merge, since am having scenario like based on domain I needs to log in different things, so can you please merge this PR sooner

@marandaneto
Copy link
Member

@marandaneto , can you please provide some approximate timeline to merge this changes to merge, since am having scenario like based on domain I needs to log in different things, so can you please merge this PR sooner

I left a few comments and specifically a different approach for compatibility, described here, would you apply those suggestions so we can move forward?

@Mastersam07
Copy link
Author

@marandaneto , can you please provide some approximate timeline to merge this changes to merge, since am having scenario like based on domain I needs to log in different things, so can you please merge this PR sooner

I left a few comments and specifically a different approach for compatibility, described here, would you apply those suggestions so we can move forward?

I will attend to these changea. I was on leave.

@marandaneto
Copy link
Member

Closed in favor of #117
Thanks for the initial PR @Mastersam07

@marandaneto marandaneto closed this Oct 2, 2024
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.

5 participants