Saw some valiant, but still wrong, forms-related JavaScript in the wild today:[1]
var mktoBinded = false,
mktoBindRetries = 0,
bindMktoSubmit = function() {
if (
"undefined" !== typeof MktoForms2 &&
MktoForms2.allForms().length > 0
) {
mktoBinded = true;
for (var a = MktoForms2.allForms(), b = 0, d = a.length; b < d; b++) {
console.log('adding GA submit event to form ' + a[b].getId() );
// stuff that adds GA send event here
}
} else {
verifyIfBinded();
}
},
verifyIfBinded = function() {
setTimeout(function() {
!mktoBinded &&
mktoBindRetries < 8 &&
(mktoBindRetries++, bindMktoSubmit());
}, 1000);
};
document.addEventListener("DOMContentLoaded", function() {
bindMktoSubmit();
verifyIfBinded();
});
The author of this code knows JavaScript in general, no question.
Problem is, they don't quite understand how the Marketo Forms 2.0 JS API works, or more specifically, how the Forms API does or doesn't signal the current form state.
The deliverable
What the code is clearly designed to do (you get a feel for these things) is:
- Wait for the Marketo Forms 2.0 library to finish loading.
- Wait for all the Marketo forms in the page to finish rendering.
- Add a Google Analytics event to each form's
onSubmitto log the form fill.[2]
If you're paying attention, you may wonder why you need to do any of this at all.
Normally, you just add a <script> that loads the Forms 2.0 library (forms2.min.js) in the <head> (or anywhere in the page, as long as it's before you try to use the Forms API) and it works fine. The usual form embed code doesn't need to do anything with a timer loop, for example: it just puts the library first, then the call to MktoForms2.loadForm() after.
The reason special code is necessary for this site in particular is that they made the twin bad (IMO) decisions to (1) load their custom onSubmit form behaviors using Google Tag Manager (GTM) and to (2) load gtm.js before the Forms 2.0 forms2.min.js is loaded.
Custom form behaviors always depend on the existence of the MktoForms2 global object. But the <script>s with the behaviors are loaded asynchronously by GTM (as with everything in GTM). So they may not be able to run immediately after loading. Instead, the code above checks if MktoForms2 exists, exits if not, and schedules a retry for 1 second later. (They check 8 times, with 1 second between checks, before failing completely. But these numbers are chosen arbitrarily: could be 500ms for 5 minutes, it's still guesswork.)
Now, the need to jump through at least one hoop may not be the developer's fault. They may be forced to use GTM by some policy set elsewhere in the business.
But the problems are:
- While they correctly detect
MktoForms2with the check-and-retry method, the next thing they do is incorrect. - They don't need to do this check-and-retry stuff at all.
Why it's broken
We're not going to change the underlying loading model (loading GTM before loading Forms 2.0). And — for the moment — let's accept that check-and-retry is necessary. These may be questionable decisions, but they're not the bug per se.
But look at how the code checks if it's good-to-go:
if (
"undefined" !== typeof MktoForms2 &&
MktoForms2.allForms().length > 0
)
Condition 1: Is MktoForms2 defined? OK, that makes sense.
Condition 2: Does the MktoForms2.allForms() array have at least one item? Oops, that doesn't make sense.
See, the allForms() array gets gradually filled in with the all the forms on the page, as they load. Merely having one item doesn't mean it's done being populated.
So if all forms on the page don't finish loading within the same second (no reason to expect this, by the way) the code fails to attach Google Analytics events properly. It doesn't throw a visible error, mind you, it just finishes before it covers all your cases.
(Not sure if there's a term of art for this type of bug, but there should be, like “early exit condition” or something.)
The quick fix
What the developer was missing is the MktoForms2.whenReady event automatically runs your desired function when every form loads, without missing any forms. It could be 5 minutes between forms and the whenReady will run for both of them.
So what it should've looked like was this:
var mktoBinded = false,
mktoBindRetries = 0,
bindMktoSubmit = function() {
if (
"undefined" !== typeof MktoForms2
) {
mktoBinded = true;
MktoForms2.whenReady(function(form){
console.log('adding GA submit event to form ' + form.getId());
// stuff that adds GA send event here
});
} else {
verifyIfBinded();
}
},
verifyIfBinded = function() {
setTimeout(function() {
!mktoBinded &&
mktoBindRetries < 8 &&
(mktoBindRetries++, bindMktoSubmit());
}, 1000);
};
document.addEventListener("DOMContentLoaded", function() {
bindMktoSubmit();
verifyIfBinded();
});
Now it won't miss nothin'.
Even better fix
Hiding in plain sight was an even better way to do this.
The developer must've missed that when DOMContentLoaded fires (they're already using this event) all synchronous <script> tags are guaranteed to be finished loading.
In other words, this is mad overkill:
document.addEventListener("DOMContentLoaded", function() {
bindMktoSubmit();
verifyIfBinded();
});
Because at the point when bindMktoSubmit() runs it's already guaranteed that the forms2.min.js is loaded and the MktoForms2 object exists!
All they ever needed were these 3 lines, with no check-and-retry loop!
document.addEventListener("DOMContentLoaded", function() {
MktoForms2.whenReady(function(form){
console.log('adding GA submit event to form ' + form.getId());
// stuff that adds GA send event here
});
});
This is an object lesson in how long and complex code isn't necessarily the right code, even if it's otherwise well-constructed. The author might just've missed the simpler way.
Notes
[1] Simplified the code to highlight the break and the fix. Also, I would've used bound instead of binded but that's probably a native language thing.
[2] Technically, they didn't add the GA send() call in the right place, but that's — if you're generous — off-topic for today.