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

Mixpanel analytics #3886

Merged
merged 8 commits into from
Feb 12, 2023
Merged

Mixpanel analytics #3886

merged 8 commits into from
Feb 12, 2023

Conversation

sirpy
Copy link
Contributor

@sirpy sirpy commented Feb 2, 2023

Description

  • Add support for mixpanel
  • modify app_open event
  • add some user props for analytics

@vercel
Copy link

vercel bot commented Feb 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
good-dapp ❌ Failed (Inspect) Feb 12, 2023 at 8:28AM (UTC)
goodid ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 12, 2023 at 8:28AM (UTC)

@johnsmith-gooddollar
Copy link
Collaborator

  1. small formatting
  2. getting source / platform repeats in analytics and routerselector. I've model it to utility method could be re-used from both places. Also I've added check for successful amplitude initialisation before call for identify
  3. amplitude and mix panel initialisations are looking somilar but not exact. I've aligned them, moving some extra stuff like logging and updating isAmp/MixpanelEnabled flags inside init methods to make initAnalytics() shorter
  4. mix panel native wrapper won't work. as you're using the MixPanelAPI object itself (not the result init() method returns) And this object exposes just init(), so other method calls will fail. To align interfaces I've generally using Proxy (see AsyncStorage or other analytics api wrappers), so I reworked it that way. Also mutating prototypes of some base classes is an anti-patter, should be avoided
  5. to avoid recall with ...args in a every method, I've also used Proxy inside Mixpanel's web api wrapper. Also it's possible to use a simple methods names map instead of declaring each method
  6. some bug fixes from local testing. there was one more misalignment - native init() is async while web's - no. I've wrapped it inside async method

@sirpy
Copy link
Contributor Author

sirpy commented Feb 2, 2023

NO REFACTORING
REVERT

@johnsmith-gooddollar
Copy link
Collaborator

@sirpy reverted

What I've left:

  1. DRY issue fix - getting user source / platform moved to util fn
  2. native wrapper fix, no prototype override
  3. if success check on amplitude init
  4. I've added param names to wrappers instead of ...params

@sirpy sirpy force-pushed the mixpanel-analytics branch from f9d6bfd to f8a0f54 Compare February 5, 2023 15:13
@sirpy sirpy dismissed johnsmith-gooddollar’s stale review February 5, 2023 15:14

need to re-review after revert

src/lib/analytics/AnalyticsClass.js Show resolved Hide resolved
src/lib/analytics/AnalyticsClass.js Outdated Show resolved Hide resolved
src/lib/analytics/AnalyticsClass.js Show resolved Hide resolved
src/lib/analytics/AnalyticsClass.js Show resolved Hide resolved
src/lib/analytics/apis.native.js Show resolved Hide resolved
src/lib/analytics/apis.native.js Show resolved Hide resolved
src/lib/analytics/apis.web.js Show resolved Hide resolved
src/lib/analytics/AnalyticsClass.js Show resolved Hide resolved
Co-authored-by: johnsmith-gooddollar <89783679+johnsmith-gooddollar@users.noreply.github.com>
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