search

koa-health / mixpanel_analytics

A dart wrapper on the mixpanel REST API to be used in Flutter applications.
https://pub.dev/packages/mixpanel_analytics
MIT License
19 stars 26 forks source link

A required stream is not the best way to set the user id #2

Closed archie-sh closed 3 years ago

archie-sh commented 5 years ago

I have a flutter project very similar to the invoice ninja app

https://github.com/invoiceninja/flutter-client/tree/develop

I am using redux and have no dependency injection framework. The authentication information is saved in an authstate that is not part of a widget, here is an example https://github.com/invoiceninja/flutter-client/blob/develop/lib/redux/auth/auth_state.dart

Using a StreamController for the userId makes it so that I have to bind the mixpanel instance to a widget. It would be helpful to be able to set the userId at a later point and not when creating the mixpanel object. Examples of this behaviour are firebase analytics and mixpanel in their native sdk:

https://firebase.google.com/docs/analytics/userid https://developer.mixpanel.com/docs/android#section-managing-user-identity

robertohuertasm commented 3 years ago

@archie-sh It's been a while since this PR was opened. Thanks for your contribution and sorry for this ultra late review. I think your proposition is valid. Are you still interested in this change? I would propose a small change to your proposal that would be to not disclose the userId but to provide a setUserId method instead. Would that work for you?

robertohuertasm commented 3 years ago

@archie-sh I took the liberty to refactor the code accordingly. @Oriol-GG are you ok with this change? I think it doesn't break anything and it allows more flexibility.