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 improvements #115

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

JuroOravec
Copy link

Hi, here's a bunch of suggestions / improvements:

Background

When I was using Mixpanel with analytics package, I wanted to use @analytics/mixpanel package with it to simplify the setup.

While it did the job, I had to disable 2 hooks on @analytics/mixpanel simply because the implementation was too opinionated.

The issue was that @analytics/mixpanel loads Mixpanel strictly via a browser script, while we were using the mixpanel-browser package. To work around that, we had to assign the mixpanel instance to window.

Another issue with the initialization was that it did not allow any mixpanel config to be passed through. Although we could use mixpanel.set_config, we needed to set the api_host from the very beginning. If we used mixpanel.set_config, this was not guaranteed.

Other issue was that the page hook implementation was too limited (only search property allowed), and having event name based on current path is an issue in downstream analytics, because you don't have a way to identify all page events (better solution is to have a constant event name, like 'page', and pass the current path as an event attribute.

So this MR introduces changes that to fix those issues, plus improve usability around specifying how the mixpanel instance should be loaded.

For individual changes, see the commits.

Changes

  • Add pageEvent mixpanel plugin config option

    • The page event name can be changed with a pageEvent plugin config value.
    • Allows to specify the name of the event that's tracked when page hook is called. Defaults to 'page'
  • Refactor page hook to pass all properties to mixpanel.track

  • Try to load mixpanel from 'mixpanel-browser' if available

    • If 'mixpanel-browser' npm package is installed, the @analytics/mixpanel
      plugin will try to use that if a mixpanel instance doesn't exist yet, otherwise it uses the load script as before
  • Move the mixpanel load script to separate file

    • Modify the load script to be able to specify which context object the mixpanel instance should be be loaded to. Defaults to window (same as before).
  • Add mixpanel mixpanel plugin config option

    • To be able to pass a mixpanel instance directly to
      mixpanel plugin instead of always fetching and initializing it from mixpanel CDN
  • Add context mixpanel plugin config option

  • Add context mixpanel plugin config option that specifies the context
    that's passed the refactored load script

  • Update hooks to get mixpanel instance from resolveMixpanel function
    instead of from window. This decouples the use of mixpanel instance
    from the window object

  • Add config mixpanel plugin config option to pass Mixpanel config

    • The config is passed to mixpanel.init if the instance needs to be
      instantiated, or to mixpanel.set_config if the instance already exists
  • Add 'getMixpanel' plugin method that returns current mixpanel instance, instead of accessing the instance via window

Juro Oravec added 7 commits November 15, 2020 12:25
- Move the mixpanel load script to separate file
- Modify the load script to be able to specify which context object
the mixpanel instance should be loaded. Defaults to `window` for
backward compatibility
- Add `context` mixpanel plugin config option that specifies the context
that's passed the refactored load script
- Update hooks to get mixpanel instance from resolveMixpanel function
instead of from `window`. This decouples the use of mixpanel instance
from the window object
- Add option to be able to pass a mixpanel instance directly to
mixpanel plugin instead of fetching it from mixpanel CDN
- Add option to pass Mixpanel config
- The config is passed to mixpanel.init if the instance needs to be
instantiated
- The config is passed to mixpanel.set_config if the instance already
exists
- Add option `pageEvent`, which allows to specify the name of the event
that's tracked when mixpanelPlugin.page is called. Defautlts to 'page'
…able

- If 'mixpanel-browser' npm package is installed, the @analytics/mixpanel
plugin will try to use that if a mixpanel instance doesn't exist yet
@rickysullivan
Copy link

The idea of having mixpanel-browser installed via NPM is a great idea. I really didn't want to have extra requests to a CDN for this to work. @DavidWells any objections to having this merged in?

@mossypaul
Copy link

Hey,
Any update on getting mixpanel-browser via NPM rather than extra request to CDN?

(I'm using vercel rewrites for mixpanel proxy, and had to add customScriptSrc point to local url for vercel to then rewrite to the CDN, so would be nice to not have to do this, and reduce an extra call)

Thanks for the package though, seems great so far

@nuttyartist
Copy link

nuttyartist commented Apr 12, 2022

Hey, Any update on getting mixpanel-browser via NPM rather than extra request to CDN?

(I'm using vercel rewrites for mixpanel proxy, and had to add customScriptSrc point to local url for vercel to then rewrite to the CDN, so would be nice to not have to do this, and reduce an extra call)

Thanks for the package though, seems great so far

@mossypaul Can you please share exactly how you used vercel rewrites with this library for Mixpanel proxy?

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.

None yet

4 participants