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

Doesn't Actually Send Data #6

Closed
seanadkinson opened this issue Nov 19, 2015 · 13 comments
Closed

Doesn't Actually Send Data #6

seanadkinson opened this issue Nov 19, 2015 · 13 comments

Comments

@seanadkinson
Copy link

I think this library doesn't actually do the right thing. Real issue being the goofy way Segment has done their JS. When the analytics script is loaded, it doesn't actually fix the initial window.analytics, it just replaces it, and the fact that this module exports a reference to the initial object, means that after the script is loaded, events aren't actually sent to Segment and tracked.

I could be wrong, but that is what I was observing.

Here is the module I created to fix this issue:

import loadAnalytics from 'analytics.js-loader';

/*
 * The analytics.js-loader module returns the "initial" analytics
 * object, which is basically just a queue to hold events until the
 * "real" one is loaded with a global script.  Problem is, the "real"
 * one simply replaces `window.analytics`, and doesn't change over
 * this initial "queue" object, meaning that nothing gets registered
 * after the "real" one is loaded - things just keep adding to the queue.
 * This is why we save off `initialAnalytics`, and then always run
 * methods through `window.analytics`, which might be the queue, and
 * might be the real one.
 */
const initialAnalytics = loadAnalytics({
    writeKey: 'KEY',
    skipPageCall: true
});
window.analytics = initialAnalytics;

const analytics = {};
_.each(initialAnalytics.methods, method => {
    analytics[method] = function() {
        return window.analytics[method].apply(window.analytics, arguments);
    };
});

export default analytics;
@vvo
Copy link
Owner

vvo commented Nov 20, 2015

I am no more using this project, cc @contra I believe you are using it and it works right? Can you have a quick look at what @seanadkinson said? thx

@yocontra
Copy link
Collaborator

Here is my code, using my fork (https://github.com/contra/analytics.js-loader):

loadAnalytics = require 'analytics.js-loader'

key = 'your segment key'
analytics = loadAnalytics key

if analytics?
  console.log '[Analytics] Loaded'
  module.exports = ->
    loadAnalytics key
else
  console.log '[Analytics] Failed to load'
  module.exports = null

@vvo
Copy link
Owner

vvo commented Nov 26, 2015

@contra Can you do a PR if you have more fixes? Actually you are now owner of both the github and npm repo so you can do anything you want here:)

@mmahalwy
Copy link

mmahalwy commented Jan 7, 2016

@seanadkinson same problem here - its not actually working...

@seanadkinson
Copy link
Author

@mmahalwy Try my code above... it has been working well for me.

@vvo
Copy link
Owner

vvo commented Jan 8, 2016

Are you guys saying the currently published module is not working at all? Would you mind submitting a patch, @seanadkinson ?

@ghost
Copy link

ghost commented Feb 1, 2016

I encountered the same issue, it does appear the currently published module doesn't work for the reasons that @seanadkinson outlined. His module works perfectly for me. Would be happy to see this integrated into the library.

@vvo vvo closed this as completed in 5871aa7 Feb 1, 2016
vvo pushed a commit that referenced this issue Feb 1, 2016
Modify load function to compensate for queueing (Fixes #6)
@ghost
Copy link

ghost commented Feb 3, 2016

Thanks for merging @vvo!

@ghost
Copy link

ghost commented Feb 3, 2016

@vvo Oops, looks like I didn't tag the commit though. Mind adding the 2.1.2 tag so we can install this using npm now?

@vvo
Copy link
Owner

vvo commented Feb 3, 2016

I think I published it

@ghost
Copy link

ghost commented Feb 4, 2016

Ah, so you did, just didn't see it in GitHub tags. Thanks!

@vvo
Copy link
Owner

vvo commented Feb 4, 2016

actually there's no real need to push tags when publishing on npm, this is just a convenience/consistency for users but no need

@vvo
Copy link
Owner

vvo commented Feb 4, 2016

npm doesnt care about the github tags

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

No branches or pull requests

4 participants