search

digital-analytics-program / gov-wide-code

Provides a set of javascript files and documentation to implement web analytics on US federal websites
http://www.digital.gov/dap
102 stars 54 forks source link

YouTube event functions are defined and callable when they shouldn't be #32

Closed konklone closed 5 years ago

konklone commented 9 years ago

Right now on GSA the DAP code throws this error:

image

It means that the onYouTubePlayerAPIReady function is getting called. onYouTubePlayerAPIReady is a magic function name that the YouTube API may trigger in response to events.

GSA is not enabling YouTube event tracking, which means that videoArray_fed is not getting defined on line 694 as expected, which causes the error above.

However, DAP's onYouTubePlayerAPIReady function is still getting defined, and so is in the global namespace. This means it can be called when no other DAP YouTube information is uninitialized. But more importantly, it also means that it could overwrite other competing scripts' onYouTubePlayerAPIReady function and disrupt site behavior.

This is happening because onYouTubePlayerAPIReady is defined inside of an if conditional statement. However, functions defined using the form function nameOfFunction() inside of a conditional block in JavaScript are always defined, even if the conditional block is never executed.

image

There are better ways of compartmentalizing these functions so that they do not enter the global namespace. However, a quick way to fix this issue is to define the functions by assigning them to a variable:

image

The DAP code, overall, should avoid entering any functions into the global namespace unless this is absolutely required. In the case of onYouTubePlayerAPIReady, it is required (which is an outdated practice of the YouTube API that gives us no choice), but its definition should be made properly conditional using the method above.

ArielZamparini-REISys commented 8 years ago

Having the same problem, not only is the Youtube api out of date with the latest google api release but as mentioned the function onYouTubePlayerAPIReady needs an additional if YOUTUBE which would completely skip youtube tracking code if that option is not set.

tdlowden commented 8 years ago

Marked for review with v3.1

tdlowden commented 5 years ago

Remedied (awhile ago). Apologies for not closing earlier!