Remove jQuery dependency from google_analytics.js

It looks like you have not seen that jquery is required by the script.

I have no plans to commit this patch at all to a stable branch as I have no idea where the code comes from and re-inventing the wheel it not something I'd preferto do. I had so many issues in past and the functions .closest and .on are critical for proper functionality. I expect that we may not need the patch here at all after the AutoTrack feature goes in.

+++ b/js/google_analytics.js
@@ -100,6 +103,47 @@
+    for ( ; elem && elem !== document && elem.nodeType === 1; elem = elem.parentNode ) {

Never seen such code. No idea whaht the first ";" is doing there.

I have no idea where the code comes from and re-inventing the wheel it not something I'd preferto do. I had so many issues in past and the functions .closest and .on are critical for proper functionality. If you coded closest yourself I'm sorry to say, but I do not know you and do not trust you to write stable code that works in all browsers. I do not like to maintain this on my own and we need to be safe that it works the next 10-15 years. If you cannot prove it comes 1:1 copy/paste from jquery i have no idea how we could maintain it properly.

For what it's worth, I applied the patch from #4 at Simplytest.me. Using the Stark theme from core, I can confirm that:

1. jQuery is not being loaded
2. Google Analytics is working properly

I added the site to my GA Account, and immediately started receiving data. I've got Event data for Downloads, Messages, Outbound links and Emails. Nothing that I've tried in about two hours of kicking the tires hasn't worked.

@hass Thanks for the excellent module. Would you reconsider using @evanmwillhite 's excellent work in removing the jQuery dependency? It would be really nice to not have the performance hit.

Ask yourself - is it worth risking 400K sites for something not tested and not properly maintained? There are not answers, too.

Overall, I can see the benefit of this refactor. jQuery gives us a consistent interface to potentially inconsistent browser DOM manipulations, however most of these actions probably don't need to be preformed with jQuery. Most of these are minor nits, and could be addressed easily.

1. +++ b/js/google_analytics.js
   @@ -3,74 +3,77 @@
   +      var elementClicked = Drupal.google_analytics.getClosest(event.target, "a,area");
   

   Needs single quotes.

2. +++ b/js/google_analytics.js
   @@ -3,74 +3,77 @@
   +        var fullHref = elementClicked.href;
   

   Maybe add some documentation about these two variables and their difference.

3. +++ b/js/google_analytics.js
   @@ -100,6 +103,47 @@
   +    for (var i=0, iLen=evts.length; i<iLen; i++) {
   

   Spacing around infix operators.

4. +++ b/js/google_analytics.js
   @@ -100,6 +103,47 @@
   +    for ( ; elem && elem !== document && elem.nodeType === 1; elem = elem.parentNode ) {
   

   Like previously mentioned, this is actually somewhat confusing.

5. +++ b/js/google_analytics.js
   @@ -100,6 +103,47 @@
   +      for (var i = 0, len = selectors.length; i < len; i++) {
   

   Can we remove the sub loop here? Perhaps filter?

6. +++ b/js/google_analytics.js
   @@ -100,6 +103,47 @@
   +        if ( elem.tagName.toLowerCase() === selectors[i] ) {
   

   Consistency around spacing of ( ).

7. +++ b/js/google_analytics.js
   @@ -100,6 +103,47 @@
   +      return null;
   

   Benefit of returning null?

8. +++ b/js/google_analytics.js
   @@ -110,7 +154,7 @@
   +    return crossDomains.indexOf(hostname) > -1 ? true : false;
   

   I'm always a bit skeptical of returning a boolean from a ternary operator.

Looks like you selected the wrong state.

Minor fixes

It would be awesome to remove jQuery as a strict dependency for this module especially for mobile browsers.

To @hass' (implicit) point though, this should never get committed without tests, especially now that Drupal has JavascriptTestBase.

Patch re-rolled to current 8.x-2.x.

We need to add the dependency on the library core/domready - patch attached.

@bgilhome any possibility to re-roll the patch for current 8.x-3.x ?

That would be great!

Patch re-rolled for 8.x-3.x

Oops... Uploaded the wrong file.
Patch re-rolled for 8.x-3.x

Re-roll for 4.x. Setting back to "needs work" because it still needs tests.

Fix mistake in re-roll. Hiding old patch.

Reroll again because domready has been deprecated.

Hi @ptmkenny,
I see from your latest comment that patch is rerolled because domready is deprecated.
But i see in your patch has core/domready is adding dependency.
I believe it is added by mistakenly.
I have updated patch removing domready library