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

Update piwik.js #6791

Closed
wants to merge 3 commits into from
Closed

Update piwik.js #6791

wants to merge 3 commits into from

Conversation

fredli74
Copy link

@fredli74 fredli74 commented Dec 3, 2014

Tried implementing sendBeacon on the client side, but it seems the server side requires a different content-type? Remove "return false" from 2421 for testing.

In my experience the 500ms wait is arbitrary guesswork and is not enough for slow connections from distant places. I increased this in my own implementation to 5000ms and only use it as a fallback timeout so that the navigation do not get stuck forever.

Also the image.onready now requires image.onerror as well since piwik does not return a valid image anymore (return code 204)

refs #6641

Tried implementing sendBeacon on the client side, but it seems the server side requires a different content-type? Remove "return false" from 2421 for testing.

In my experience the 500ms wait is arbitrary guesswork and is not enough for slow connections from distant places. I increased this in my own implementation to 5000ms and only use it as a fallback timeout so that the navigation do not get stuck forever.

Also the image.onready now requires image.onerror as well since piwik does not return a valid image anymore (return code 204)
@fredli74
Copy link
Author

fredli74 commented Dec 3, 2014

please disregard from this pull request. Turns out 5000ms default expiry time is a very bad idea. I have previously been using callbacks for this but just now implemented it as a patch on the unload handler. This turns out bad as the unload handler is blocking. It will end up freezing the UI for 5s in certain situations. I will go back to the drawing board on this one. =)

5000ms timeout is bad, even the 500ms is bad because it does a blocking wait that is only based on a guess that the request might reach in time. Because of this I stopped using "enableLinkTracking" all together and implemented trackLink with callbacks on all outlinks instead.

For this to work, the callback must still be fixed, that's why I still need this patch for the "image.onerror = "
Stupid addition as the setExpireDateTime function never decreases a timeout, only extends it.
@mattab
Copy link
Member

mattab commented Dec 3, 2014

Hi @fredli74 ok I'm closing for now, feel free to create new one for #6641

@mattab mattab closed this Dec 3, 2014
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Dec 3, 2014
@fredli74 fredli74 deleted the patch-1 branch December 5, 2014 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants