alphagov / govuk_frontend_toolkit

❗️GOV.UK Frontend Toolkit is deprecated, and will only receive major bug fixes and security patches.
MIT License
403 stars 107 forks source link

Remove Google Analytics code (breaking change) #481

Closed andysellick closed 4 years ago

andysellick commented 4 years ago

This is a breaking change for any applications other than static that rely on this code

This code has already been migrated to static, but due to the non-specific way the JS is initialised it was being run twice by tests there, with incorrect results.

In other words, static's live JS was using it's own analytics code (this code, previously migrated) and then running it all again a second time from the installed toolkit code. This wasn't an issue until we started changing some of it and found that the results from the tests contradicted the analytics code in static.

Also, removing this code reduces the amount of JS static contains, thereby taking the page weight back down to where it should be.

andysellick commented 4 years ago

@nickcolley this is a change required by GOV.UK, do you know who else uses the toolkit? I'm nervous about the wider impact of removing this code, but we need to do it.

36degrees commented 4 years ago

do you know who else uses the toolkit?

Between govuk_frontend_toolkit_npm and govuk_frontend_toolkit_gem there are at least 2,250 dependencies.

It's hard to know how many of them are using the analytics functionality, but in the past we have had a good few analytics-based issues raised by users outside of alphagov.

Is there any way of making this work for GOV.UK without changing it here? For example, by updating Static to only include the JS that it needs from here?

andysellick commented 4 years ago

@36degrees thanks that's good to know. I think the problem is this line here: https://github.com/alphagov/govuk_frontend_toolkit/blob/master/javascripts/govuk_toolkit.js because that includes everything, although I'm not sure where that gets called if the toolkit is a dependency.

Unfortunately even if I were to modify that file to include only specific things, it sounds like it would still need to include that analytics code.

NickColley commented 4 years ago

I would be in favour of moving this code into GOV.UK Publishings codebase.

I think by removing it from this library it will make it easier for GOV.UK Publishing (who are the main people that contribute to this part of the codebase) to iterate on this without being blocked by this library being deprecated.

We would then recommend in the release notes for any other teams X-GOV to copy these files as well.

andysellick commented 4 years ago

Thanks @nickcolley . Since this could be a complicated decision, there is another option, which is that I could make the changes I need (and have already made in static) here as well. That way we could leave the analytics code here a bit longer for those who rely on it, and they'd only need to make a small change to their codebase to keep up with the change.

Those changes are here and it's basically only to do with the cross domain linking code - the function addLinkedTrackerDomain currently accepts a string and turns it into an array (for GA's purposes), I need it to accept an array of strings, so that I can pass more than one string.

andysellick commented 4 years ago

@36degrees @nickcolley I've updated the CHANGELOG to reflect that this is a breaking change, let me know if it's enough or I need to add more.