Skip to content

Conversation

djih
Copy link
Member

@djih djih commented Oct 18, 2016

  1. Customers have asked for SDK to automatically parse gclid url param (Google click id) and set as a user property (like what we do for UTM params already). Added includeGclid configuration option
  2. Currently the SDK will only save referrer and UTM params once per session, at the start of the session. I feel like the logic might be slightly buggy and some customers are reporting that the UTM params are not being captured. Added a saveParamsReferrerOncePerSession option that they can set to false to remove this restriction
  3. Before the SDK had this convoluted logic to implement the "once per session" restriction. I saved the UTM parameters and referrer to the browser's sessionStorage. And so if the SDK saw an existing value in the sessionStorage, then it assumed that it already ran the logic and sent the values in the current session. I feel like this could be buggy, and the browser's sessionStorage expiration window and whatever might not line up with our SDK's definition of a session. I removed the use of sessionStorage completely, and the UTM param / referrer saving logic simply just saves the values every time it is called. The init method is the one that decides when to call it depending on configuration options and if a new session is starting. This should make the logic easier to reason about, and give the customer freedom to do whatever they want.
  4. Removed a bunch of unnecessary unit tests around the convoluted logic described above. Added new tests for the override option and GCLID.
  5. Added a new README section explaining all of these options.

PTAL @ManThursday

Copy link

@trashstack trashstack left a comment

Choose a reason for hiding this comment

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

Looks good to me.


// parses the value of a url param (for example ?gclid=1234&...)
var getQueryParam = function getQueryParam(name, query) {
name = name.replace(/[\[]/, "\\[").replace(/[\]]/, "\\]");

Choose a reason for hiding this comment

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

Seems like this line doesn't get used anymore?

// parses the value of a url param (for example ?gclid=1234&...)
var getQueryParam = function getQueryParam(name, query) {
name = name.replace(/[\[]/, "\\[").replace(/[\]]/, "\\]");
var regex = new RegExp("[\\?&]" + name + "=([^&#]*)");

Choose a reason for hiding this comment

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

Seems ok but I always get a bit nervous with regex parsing. Is it worth taking into account the possibility of repeated params?

@djih djih merged commit de9acd0 into master Oct 19, 2016
@djih djih deleted the glic-config-options branch October 19, 2016 07:39
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

Successfully merging this pull request may close these issues.

2 participants