Skip to content

warn if usePost is undefined (close #613) #615

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.

Sign up for GitHub

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

Closed
Chris Sherlock (chrissherlock) wants to merge 2 commits into from
Closed

warn if usePost is undefined (close #613) #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1d72d3
warn if usePost is undefined (close #613)
chrissherlock Oct 18, 2017
947f71e
update Tracker documentation (close: #605)
chrissherlock Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/js/out_queue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,16 @@
outQueue;

// Fall back to GET for browsers which don't support CORS XMLHttpRequests (e.g. IE <= 9)
usePost = usePost && window.XMLHttpRequest && ('withCredentials' in new XMLHttpRequest());
if (typeof usePost != 'undefined')
{
usePost = usePost && window.XMLHttpRequest && ('withCredentials' in new XMLHttpRequest());
path = usePost ? '/com.snowplowanalytics.snowplow/tp2' : '/i';
}
else
{
Console.warn('usePost undefined');
path = '/i';
}

@BenFradet Ben Fradet (BenFradet) Oct 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we have

usePost = typeof usePost != 'undefined' && usePost && window.XMLHttpRequest && ('withCredentials' in new XMLHttpRequest());

What do you think Anton Parkhomenko (@chuwy) ?

@chrissherlock Chris Sherlock (chrissherlock) Oct 19, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see a warning. One of the orgs who uses this has managed to encode an absolutely massive object and because this variable is undefined they are using GET requests... and thus is getting 414 errors because the URI is enormous... if they picked up on the fact that they hadn't set usePost, then it would have alerted them to the issue more quickly.

@chuwy Anton Parkhomenko (chuwy) Oct 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if I understand it right - if website-owner just wants to use GET-requests with reasonable payload - we'll force all of his users see "usePost undefined" message in console, which doesn't look right for me. So, this change is not about undefined-check it's more about GET-check. But correct me if I'm wrong.

I'm quite against all log messages from tracker - in 99,9% of cases they hit console-aware users and not website-owners. But I'm quite keen to introduce something like debug-mode for tracker, where all such cases can be safely printed in development-environment and deleted in production.

If we want to avoid 414-like issues - all we can do is to be more explicit in tracker documentation.

@chuwy Anton Parkhomenko (chuwy) Oct 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to fix it would be to use POST by default and allow to use GET by settings useGet = true, but this is quite big change for me, taking in account that CloudFront collector doesn't support POST.


var path = usePost ? '/com.snowplowanalytics.snowplow/tp2' : '/i';

Expand Down
32 changes: 15 additions & 17 deletions src/js/tracker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,23 +66,21 @@
* 4. appId, ''
* 5. platform, 'web'
* 6. respectDoNotTrack, false
* 7. userFingerprint, true
* 8. userFingerprintSeed, 123412414
* 9. pageUnloadTimer, 500
* 10. forceSecureTracker, false
* 11. forceUnsecureTracker, false
* 12. useLocalStorage, true
* 13. useCookies, true
* 14. sessionCookieTimeout, 1800
* 15. contexts, {}
* 16. post, false
* 17. bufferSize, 1
* 18. crossDomainLinker, false
* 19. maxPostBytes, 40000
* 20. discoverRootDomain, false
* 21. cookieLifetime, 63072000
* 22. stateStorageStrategy, 'cookieAndLocalStorage'
* 23. respectOptOutCookie, false
* 7. userFingerprintSeed, 123412414
* 8. pageUnloadTimer, 500
* 9. forceSecureTracker, false
* 10. forceUnsecureTracker, false
* 11. useLocalStorage, true
* 12. useCookies, true
* 13. sessionCookieTimeout, 1800
* 14. contexts, {}
* 15. post, false
* 16. bufferSize, 1
* 17. crossDomainLinker, false
* 18. maxPostBytes, 40000
* 19. discoverRootDomain, false
* 20. cookieLifetime, 63072000
* 21. stateStorageStrategy, 'cookieAndLocalStorage'
*/
object.Tracker = function Tracker(functionName, namespace, version, mutSnowplowState, argmap) {

Expand Down