-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
JS Tracker api: add ping() method #14127
Conversation
js/piwik.js
Outdated
* length, ping requests will create a new visit using the last action in the last known visit. | ||
*/ | ||
this.doPing = function () { | ||
trackRequest('ping=1', null, null, 'ping') |
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.
This should be this.trackRequest
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.
Also I wonder if we should rename it to maybe trackPing()
as we usually name things like this trackGoal, trackPageview, ...
or maybe sendPing()
as it's not really tracking anything.
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.
I picked doPing
to match the PHP API, but could easily change it. I agree though that it should use a 'neutral' verb to be clear that it doesn't generate any sort of tracking event.
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.
Good thinking. The JS tracker and PHP tracker do have bit different naming style though. Eg PHP tracker always adds the do
which JS tracker doesn't. Better to maybe stay consistent within JS tracker and not use do
.
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.
Okay, then how about just ping
?
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.
sure 👍
js/piwik.js
Outdated
@@ -4526,8 +4526,7 @@ if (typeof window.Piwik !== 'object') { | |||
heartBeatPingIfActivityAlias = function heartBeatPingIfActivity() { | |||
var now = new Date(); | |||
if (lastTrackerRequestTime + configHeartBeatDelay <= now.getTime()) { | |||
var requestPing = getRequest('ping=1', null, 'ping'); | |||
sendRequest(requestPing, configTrackerPause); | |||
doPing(); |
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.
This should be trackerInstance.doPing()
Cheers for this @pimlottc-gov Left a few comments. Ideally you also generate the up to date minified version see https://github.com/matomo-org/matomo/tree/3.x-dev/js#deployment and add an entry to the developer changelog but I suppose we could do this as well. |
Thanks for the instructions, I went ahead and added a minified build. I'm not sure, though, which changelog to update? |
I've added the changelog entry and potentially fixed a test which doesn't liked a newline at the end. Will keep an eye on whether tests pass. Cheers for this 👍 |
Cheers @pimlottc-gov for this PR 👍 |
Fixes #14100