Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Current Problem
Currently, this module adds jQuery as a dependency for administration, debugging and the public tracking script. The flexibility in Drupal 8 allows us to control that per library, and the simplicity of the public-facing script lends itself to removing that dependency. This would allow non-administrative and non-developer traffic to lighten their load and execution time when loading/running js/google_analytics.js.
Details
- Javascript changes will only impact js/google_analytics.js
- Make use of core domready
- Leverage modern javascript solutions that fit the Drupal 8 browser requirements
Comments
Comment #2
hass CreditAttribution: hass commentedIt looks like you have not seen that jquery is required by the script.
Comment #3
evanmwillhite CreditAttribution: evanmwillhite commentedI did see it's a requirement currently, and I'm working to rewrite the javascript to remove that requirement. I've changed the Category to "Feature Request" though, as I think that makes more sense. Works fine as-is, this would just be nice to have.
Comment #4
evanmwillhite CreditAttribution: evanmwillhite commentedI've now edited js/google_analytics.js to be vanilla javascript and removed the dependency in google_analytics.libraries.yml.
Comment #5
evanmwillhite CreditAttribution: evanmwillhite commentedComment #6
hass CreditAttribution: hass commentedComment #7
evanmwillhite CreditAttribution: evanmwillhite commented@hass can we consider not postponing this? The autotrack issue won't resolve all of what is being handled in this script (e.g., mailto, downloads and colorbox), and I think this file being vanilla js would actually be a step toward playing well with autotrack, which is also vanilla js. It's also just a minor tweak to get it inline with Drupal 8 best practices. Implementing a library like Autotrack is a much larger task.
Comment #8
hass CreditAttribution: hass commentedI 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.
Never seen such code. No idea whaht the first ";" is doing there.
Comment #9
evanmwillhite CreditAttribution: evanmwillhite commentedI believe a couple of items here need clarity: 1. the Autotrack association and 2. code/syntax and testing.
The Autotrack association
If the Autotrack library was implemented today, you would still need the majority of js/google_analytics.js, and that code currently is dependent on jQuery code. My changes would still stand and help to remove that dependency. Below is a list of the Autotrack functionality using the following format: PLUGIN_NAME (RELATIONSHIP TO JS/GOOGLE_ANALYTICS.JS): DESCRIPTION_OF_PLUGIN
In other words, below are the parts of js/google_analytics.js that would still be required even if the Autotrack plugin was used:
In summary, I don't believe this should be seen as a child to the Autotrack issue.
Code syntax and testing
I want this javascript to be tested by the community. I have tested it myself manually in the D8 supported browsers, but I would expect and hope for feedback and assistance in making sure it is readable and functional. To answer your specific question, that semicolon at the opening of the for loop just means that the initial expression isn't required in this instance. Here's a good explanation.
But please hear me when I say I never expected this code to just be merged into the stable branch outright. I was hoping that it would stay in "Needs Review" until other javascript developers in the community had a chance to vet it. Is that a possibility?
Comment #10
evanmwillhite CreditAttribution: evanmwillhite commented@hass Have you had a chance to read through what I wrote above? Basically, the ideal situation is that this would be moved back to "Needs Review" so that it can get some community testing and vetting.
Comment #11
hass CreditAttribution: hass commentedI 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.
Comment #12
UpTil4Music CreditAttribution: UpTil4Music commentedFor what it's worth, I applied the patch from #4 at Simplytest.me. Using the Stark theme from core, I can confirm that:
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.
Comment #13
hass CreditAttribution: hass commentedAsk yourself - is it worth risking 400K sites for something not tested and not properly maintained? There are not answers, too.
Comment #14
GrandmaGlassesRopeManOverall, 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 withjQuery
. Most of these are minor nits, and could be addressed easily.Needs single quotes.
Maybe add some documentation about these two variables and their difference.
Spacing around infix operators.
Like previously mentioned, this is actually somewhat confusing.
Can we remove the sub loop here? Perhaps filter?
Consistency around spacing of
( )
.Benefit of returning
null
?I'm always a bit skeptical of returning a boolean from a ternary operator.
Comment #15
hass CreditAttribution: hass commentedLooks like you selected the wrong state.
Comment #16
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedMinor fixes
Comment #17
krlucas CreditAttribution: krlucas commentedIt 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.
Comment #18
bgilhome CreditAttribution: bgilhome commentedPatch re-rolled to current 8.x-2.x.
Comment #19
bgilhome CreditAttribution: bgilhome commentedWe need to add the dependency on the library core/domready - patch attached.
Comment #20
sahaj CreditAttribution: sahaj commented@bgilhome any possibility to re-roll the patch for current 8.x-3.x ?
That would be great!
Comment #21
kunal_singh CreditAttribution: kunal_singh at Innoraft for Drupal India Association commentedPatch re-rolled for 8.x-3.x
Comment #22
kunal_singh CreditAttribution: kunal_singh at Innoraft for Drupal India Association commentedOops... Uploaded the wrong file.
Patch re-rolled for 8.x-3.x
Comment #23
ptmkenny CreditAttribution: ptmkenny commentedRe-roll for 4.x. Setting back to "needs work" because it still needs tests.
Comment #24
ptmkenny CreditAttribution: ptmkenny commentedFix mistake in re-roll. Hiding old patch.
Comment #25
ptmkenny CreditAttribution: ptmkenny commentedReroll again because domready has been deprecated.
Comment #26
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedHi @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
Comment #27
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commentedComment #28
Megha_kundar CreditAttribution: Megha_kundar at Srijan | A Material+ Company for Drupal Association commented