Add support for Google Analytics fieldsObject on create

[colinrotherham]

Like the cookieDomain field, other fields are “Create Only”:

1. sampleRate
2. siteSpeedSampleRate
3. allowLinker

See the reference guide here:
https://developers.google.com/analytics/devguides/collection/analyticsjs/field-reference

Currently cookieDomain is hard-coded into the GA create call:
ee2fd0d#diff-8b93af583c1a7ca5e9c48a89ebce2b38R10

This means none of the fields above can be added 😢

Extensibility

To expand on this, I've added a new fieldsObject parameter to these methods:

new GOVUK.Analytics(config, fieldsObject)
new GOVUK.GoogleAnalyticsUniversalTracker(id, cookieDomain, fieldsObject);

This change now allows the GOV.UK analytics modules to call:

ga('create', 'id', {
  cookieDomain: 'example.com',

  // New fields now supported
  siteSpeedSampleRate: 100,
  allowAnchor: true
});

Thanks

[colinrotherham]

Hi @robinwhittleton,

Not sure if an unsolicited pull request was the right approach, but would be great to have these tweaks reviewed.

Thanks.

[NickColley]

Are we sure current instances match this new source? If we need both maybe we should select both.

e.g. I'm thinking the current code probably looks like

<script src="//www.google-analytics.com/analytics.js"></script>

[colinrotherham]

Hi @NickColley,

Technically this line is redundant. Until GOVUK.GoogleAnalyticsUniversalTracker.load() runs there's no Google Analytics script in the DOM.

I presume at some point it was a "clean up" before the test runs?
We can remove it if you like.

Otherwise yes, the new instances will always be https:// as shown here:
https://github.com/alphagov/govuk_frontend_toolkit/pull/298/files#diff-8b93af583c1a7ca5e9c48a89ebce2b38R30

[NickColley]

Hey @colinrotherham sorry for the lack of response :) Thanks for your PR.

I'm not so familiar with this codebase yet but could we pass these fieldObjects via the config?

[colinrotherham]

@NickColley Yeah course, pick which you prefer and I can implement it. The conundrum was not having a consistent approach already.

For example, originally:

This constructor took two fixed arguments:

new GOVUK.GoogleAnalyticsUniversalTracker(id, cookieDomain);

This constructor took a single config object:

new GOVUK.Analytics(config);

Ideally both should use a config object but it'll mean breaking existing implementations. For safety, I added the extra parameter fieldsObject to both.

If I follow a "When in Rome" approach I can use the config object for GOVUK.Analytics? We'll still need a 3rd argument for GOVUK.GoogleAnalyticsUniversalTracker to retain compatibility.

Let me know and I can amend.

[colinrotherham]

@NickColley I've gone ahead and combined fieldsObject into the GOVUK.Analytics() config argument. It made good sense.

[colinrotherham]

Current boilerplate:

GOVUK.analytics = new GOVUK.Analytics({
  universalId: 'UA-XXXXXXXX-X',
  cookieDomain: cookieDomain
});

The following fieldsObject is now supported:

GOVUK.analytics = new GOVUK.Analytics({
  universalId: 'UA-XXXXXXXX-X',
  cookieDomain: cookieDomain,
  fieldsObject: {
    siteSpeedSampleRate: 100
  }
});

[robinwhittleton]

Are we missing a comment here?

[colinrotherham]

Very good point. Strange though, apparently this is still valid JavaScript.

[dsingleton]

Hi @colinrotherham,

Thanks for the contribution. An unsolicited PR is fine, and a good way to discuss things in the open 👍

Being able to control more of the GA fields, like the ones you mentioned, but in theory any of them is great. I'm taking a proper look at config vs fieldsObject now, as it's been a while since i've touched this JS.

Ideally it'd be great if we only had one configuration object, but in a backwards compatible way. Could we merge the values of fieldsObject and config for the Analytics object?

GoogleAnalyticsUniversalTracker could take it's second argument as a config object, but support a string for backwards compatibility. What do you think?

[colinrotherham]

Hi @dsingleton.

Yeah if you're happy to?? Would be great for consistency.

Ok so I've aligned both constructors so they take the same params:

new GOVUK.Analytics(trackingId, fieldsObject);
new GOVUK.GoogleAnalyticsUniversalTracker(trackingId, fieldsObject);

This keeps us consistent with Google Analytics too:

ga('create', trackingId, fieldsObject);

I've updated the tests to prove everything still works for:

1. Old GOVUK.Analytics syntax (unofficial universalId parameter)
2. Old GOVUK.GoogleAnalyticsUniversalTracker syntax (strings for id and cookieDomain)

This keeps the new fieldsObject params compatible with Google parameter names, plus we've got a shim in each constructor to check for the old parameter types.

[colinrotherham]

Current boilerplate:

GOVUK.analytics = new GOVUK.Analytics({
  universalId: 'UA-XXXXXXXX-X',
  cookieDomain: cookieDomain
});

New combined boilerplate

GOVUK.analytics = new GOVUK.Analytics('UA-XXXXXXXX-X', {
  cookieDomain: cookieDomain,
  siteSpeedSampleRate: 100
});

No more unofficial universalId property. This didn't match the Google Analytics API docs.

[colinrotherham]

Hi @dsingleton, have you had another look?

[dsingleton]

Hi @dsingleton, have you had another look?

Hi @colinrotherham - Sorry, i've come back to look at this a couple of times today - it's quite a dense one :)

Generally, 👍 . I'm most concerned with the API changes - the internal changes are well tested, and work, and we can always change them later. Getting the API right is hard, but worth a bit of pain, so apologies for the back and forth :)

No more unofficial universalId property. This didn't match the Google Analytics API docs.

For context, originally the universalId property was in config so we could support classic and universal during a migration, with Analytics masking the difference. I'd be inclined to keep that abstraction, only passing config to the constructor, giving us more flexibility for the future. As it's an abstraction over GA, i'm less worried about unofficial properties, and not mirroring the GA API exactly.

It's not the strongest opinion though, but i'm inclined to stick with what we have now for Analytics - a single config argument - as a default. What do you think?

But i'm very 👍 to the change to GoogleAnalyticsUniversalTracker to mirror the GA API with trackingID and fields.

[colinrotherham]

Thanks for the review @dsingleton.

I took the approach of a new developer, a new service team or someone new to the Analytics modules and what would make sense to them.

It's important to remember that there is no Universal ID to Google.

i.e. Whether you're on the old Google Analytics ga.js or new Universal Analytics analytics.js, it's the same Tracking ID number for both. Both libraries require it—it's not just an abstraction for Universal Analytics.

For newer developers to GOV.UK (and new teams) like me, these changes mean we now see documentation consistent with that provided by Google. It makes sense, and both legacy and universal modules are in parity.

I appreciate the history of universalId, but the universal approach is no longer optional. The old ga.js library is considered "legacy" and not for new accounts: https://developers.google.com/analytics/devguides/collection/upgrade/

Learning and Familiarity

Developers who are unfamiliar with Google Analytics don't need to know about the legacy API and ga.js library. It's no longer offered to new accounts. It's not an alternative.

I could put universalId back into the config object, but behind the scenes the module would need to remove it before passing it on to Google. It becomes a deceptive API, not doing what it appears to do.

Developers start learning two competing systems, rather than two in unity.

[colinrotherham]

Hi @dsingleton, do you need anything else from me here before we settle on direction?

[dsingleton]

Hi @dsingleton, do you need anything else from me here before we settle on direction?
Show all checks

Hey,

I totally understand your reasoning for the Analytics API to mirror GA, and the feedback is really useful. Right now, that doesn't match how we're using the Analytics class - changing it feels like a more significant thing and we'd want to think about in more detail.

IMO the best way to settle this is treat it as two separate issues, the improved functionality, and the more contentious semantics.

I'd be more than happy to merge the rest of this PR if it doesn't include the Analytics API change - the extra configurability is a great feature.

If you still think the Analytics API should mirror GA then how about opening a fresh PR/issue to discuss that separately? I think that'd make it easier to discuss the change, separate from a functionality change.

Does that sound reasonable?

ps: It shouldn't normally take 12 days to get a PR sorted, we should be doing better. I've been in and our of the office quite a bit recently, so that's entirely my fault. I'm sorry 😦

[colinrotherham]

Thanks @dsingleton,

That's definitely reasonable, I've gone back to the old params for GOVUK.Analytics but retained the fieldsObject change to GOVUK.GoogleAnalyticsUniversalTracker.

I can make a new pull request once merged in.

[dsingleton]

🎉

[NickColley]

Thanks @colinrotherham 👍 💯

[colinrotherham]

Excellent, thanks @dsingleton @NickColley.

What are the next steps for a GitHub release tag and publish to npm?

[dsingleton]

@colinrotherham The release process is to open a PR that bumps the version number and updates the CHANGELOG, this PR is a good example #302

Once the version change PR is merged it'll trigger a build of a new version and push it to rubygems, npm, etc.

[robinwhittleton]

Discussing at the moment what the version number should be and whether we want to get anything else into the same release before publishing.

[dsingleton]

My $0.02: it adds new functionality, but in a backwards compatible way - so a minor version bump.

[colinrotherham]

@robinwhittleton @dsingleton Let me know when you've decided. I presume if you're bundling other feature you won't need me to create the pull request.

Thanks

[robinwhittleton]

@colinrotherham this was released in 4.15.0. Thanks for your contribution!