-
Notifications
You must be signed in to change notification settings - Fork 85
Conversation
2ff25cb
to
fd360ba
Compare
*/ | ||
|
||
Analytics.prototype.ready = function(a, b) { | ||
if (is.fn(a)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @segmentio/gateway what's the javascript convention here as to how to document this?
@tsholmes I think i've seen some code/PR by you that had this overloaded function behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this would be right:
@param {(Function|String)} a
@param {Function} [b]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rowno any suggestions for naming the parameters? I used a
/b
coz I couldn't think of anything better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯_(ツ)_/¯ At least add description to the parameters though. e.g:
@param {(Function|String)} a Callback function or integration name.
@param {Function} [b] Callback function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rowno updated
fd360ba
to
419df20
Compare
I think tests are failing with saucelabs (unsure if it's a test error or saucelabs error)? https://circleci.com/api/v1.1/project/github/segmentio/analytics.js-core/156/output/13/0?file=true
|
@f2prateek set the karma deps to (pinned exactly): |
419df20
to
96527ef
Compare
@tsholmes same issue I think |
96527ef
to
035552d
Compare
@tsholmes reverted the package.json changes, will tackle in a different PR |
e4e97c6
to
c4cb3df
Compare
Hi! Would love to have this as I'm experiencing the exact problem outlined in segmentio/analytics.js#488 (comment) Is this project still active? |
@elgreg yes, analytics.js is used by thousands of companies :) This particular issue has been on the back burner though. We'll try to pick it back up. |
Added to our internal tracker as well https://segment.atlassian.net/browse/LIB-103. |
@f2prateek Ha! Sorry - I was just referring to this particular PR when I asked if the project was still active. I should have been more clear. That's great news about this potentially becoming a feature! We just released our first pass at a conversion to analytics.js and are experiencing the issue referred to by some others where adBlock blocking one integration ends up blocking ready(). I'll follow this issue closely for updates! |
See segmentio/analytics.js#409 This adds the ability to set a ready callback per integration. todo: * document the function behaviour (need help here on what syntax to use) * test for already readied integration
c4cb3df
to
ca6da2f
Compare
@f2prateek sorry for resurrecting this. Ran into this today where adblocker blocked 1 integration which prevents from knowing when other integrations had loaded and are ready to use. |
Closing this PR. If this is still a problem, please reopen so that we can get it properly reprioritized. |
See segmentio/analytics.js#409
This adds the ability to set a ready callback per integration.
todo:
[ ] document the function behaviour (need help here on what syntax to use)
[ ] test for already readied integration