7
\$\begingroup\$

I want to load JS files asynchronously to speed up page loading, but also need to execute JS code only once the scripts finish loading (so I need a way to implement callbacks).

I based this off of some example code on the Google Pagespeed website:

// Add script elements as a children of the body
function downloadJSAtOnload() {
    var filesToLoad = [
        ["/assets/plugins/typeahead.bundle.min.js", "onTypeaheadLoaded"],
        ["https://apis.google.com/js/client:plusone.js?onload=googlePlusOnloadCallback", ""]
    ];

    filesToLoad.forEach(function(entry) {
        var element = document.createElement("script");
        element.src = entry[0];
        if (entry[1] != "") { // if an onload callback is present (NOTE: DOES NOT SUPPORT NAMESPACES -- http://stackoverflow.com/a/359910/1101095)
            element.onload = function() {
                if (typeof window[entry[1]] != "undefined") {
                    window[entry[1]]();
                }
            };
        }
        document.body.appendChild(element);
    });
}

// Check for browser support of event handling capability
if (window.addEventListener)
    window.addEventListener("load", downloadJSAtOnload, false);
else if (window.attachEvent)
    window.attachEvent("onload", downloadJSAtOnload);
else window.onload = downloadJSAtOnload;

I know there are libraries out there for dependency management, callback functions, etc., but I just want something very lightweight and using a library would mean having another HTTP request.

I'm very new to JavaScript and would really appreciate any feedback or advice you might have!

\$\endgroup\$
  • 2
    \$\begingroup\$ igvita.com/2014/05/20/… perhaps you consider using async \$\endgroup\$ – Thomas Junk Jul 19 '14 at 17:09
  • \$\begingroup\$ I'm really not a fan of your global variable onTypeadheadLoaded. There are many more elegant ways to do this. typeahead requires jQuery yes? why not use getScript \$\endgroup\$ – megawac Jul 19 '14 at 18:33
  • \$\begingroup\$ Also my check would probably be entry[1] != "" && typeof window[entry[1]] != "undefined" \$\endgroup\$ – megawac Jul 19 '14 at 18:36
  • \$\begingroup\$ @ThomasJunk very interesting.. That's contrary to pretty much everything I've read on the subject, but the author does provide evidence to support his position. The question is, if I use the async attribute, can I specify a callback somehow? \$\endgroup\$ – Nate Jul 19 '14 at 20:00
  • \$\begingroup\$ @megawac Interesting idea on using jQuery. I did a little searching and apparently getScript() uses eval, which I've read is bad. I also found an article which says the callback is fired onload not when the script has been parsed and is available to use, but I don't know if that's correct or not (looseideas.co.uk/getscript-callbacks-defer-async). \$\endgroup\$ – Nate Jul 19 '14 at 20:01
5
\$\begingroup\$

First of all, I would split you problem into 3 parts:

  1. Loading a js-file asynchronously, and calling a callback-function
  2. Loading several js-files an once asynchronously
  3. Using an onload-Handler, which triggers everything

These are 3 independent parts, which might be reused for other purposes, if seperated correctly.

1. Loading a js-file asynchronously

You're using 2 strings to specify a file to be loaded. But instead of writing

["file.js", "cbFunction"]

you can as well also pass the actual function:

["file.js", cbFunction]

This not only ensures that the function actually exists, it also gives you the opportunity to use anonymous functions:

["file.js", function(){ alert("loaded!"); } ]

However, speaking of segregation of responsibilities, you should write a function on its own, without the need of an Array anymore:

function loadScript(path, scriptLoadedCallback){
    var element=document.createElement('script');    //generate <script>-tag
    element.setAttribute("type","text/javascript");
    element.setAttribute("src", path);

    if(typeof(scriptLoadedCallback) == 'function'){  //makes the callback-function optional
        element.onload = function() {
            return scriptLoadedCallback(true, path); //true = successfull; the path is needed later
        };
        element.onerror = function() {               //you might also call the cb on error
            this.parentNode.removeChild(this);       //remove faulty node from DOM
            return scriptLoadedCallback(false, path);      //false = error; the path is needed later
        };
    }
    document.head.appendChild(fileref);              //insert the node in DOM (end of <head>), and load the script
}

This function looks much clearer, and you can see at first glance what it is doing.

Note that I checked the existence of the callback before I append an onload-attribute. This is a bit more efficient than your code, which always calls the onload-Handler and then does nothing.

You might also want to add the onerror-method, otherwise you will never get notified if the loading failed (This usually happens if the js-file doesn't exist).

If you only have to load a single script, you can call this function directly. No need to generate useless Arrays.

2. Loading several js-files an once

If you only want to load several files, and calling a callback after each file, this is an easy task, using your original array (with functions instead of strings):

var filesToLoad = [
    ["file1.js", cbFunctionToCall],
    ["file2.js"]
];
for(var i=0; i<filesToLoad.length; i++){ //I prefer a for-loop because its more readable
    loadScript(filesToLoad[i][0], filesToLoad[i][1];
}

And that's all.

However, if you want to have one callback-function which is called after all files have been loaded, this gets a bit tricky. You don't know which file is the last one, and which file should trigger the actual callback-function.

function loadScriptArray(contentArray, contentLoadedCallback){
    var contentQuantity = contentArray.length;      //Number of Files that needs to be loaded
    var contentCompleted = 0;                       //Number of Files, that have already been loaded
    var returnParamList = {};                       //List with return-Parameters

    if(contentQuantity == 0){                       //We don't have anything to load
        return contentLoadedCallback({});
    }

    for (var i = 0; i < contentQuantity; i++) {
        loadScript(contentArray[i], function(success, path){   //This anonymous function is called everytime a script is finished
                                                               //The only way to know which script finished, is to pass the path as an parameter
            returnParamList[ identifier ] = returnParam;       //store the returnValue (true=success, false=error)
            contentCompleted++;
            if(contentCompleted == contentQuantity){    //this was the last file
                if(typeof contentLoadedCallback== 'function'){
                    contentLoadedCallback(returnParamList);
                }
            }
        });
    }
}

This is how the function is being called:

loadScriptArray(["file1.js", "file2.js"], function(returnParamList){
    alert("All Scripts finished. \n"+
          "File1: "+returnParamList["file1.js"]+"\n"+
          "File2: "+returnParamList["file2.js"]);
});

This function loads 2 files simultaneously, and calls the callback after the last one has been loaded. Afterwards it prints which files have been loaded successfully, and which loading-operation failed.

This only works, if the initial loadScript()-Function always calls the callback-Function - both if it succeeded or failed. And to report the success back to the caller, the callback-function also needs the filePath as an parameter.

Another advantage is, that you can extend this function to also load other files like images, json, xml and so on.

3. Trigger loading with onload

You code looks fine here, with only one very bad issue: Use brackets, they won't kill you.

There's absolutely no reason to omit brackets, it's just a possible source for annoying errors, which are very hard to be found.

\$\endgroup\$
  • \$\begingroup\$ Note that I tested this function on moderns browsers only (Chrome, FF, Opera, IE). Older browsers might need special treatment. \$\endgroup\$ – maja Jul 20 '14 at 8:13
  • \$\begingroup\$ @ckuhn203 That's true. But the question seemed to me that the OP isn't sure if his approach is the best choice, so I just offered an alternative. Although it might be offtopic on codereview, it still may help the OP. \$\endgroup\$ – maja Jul 20 '14 at 11:03
  • \$\begingroup\$ @maja, your answer does touch on some important issues with the code in the OP (separation of concerns, taking advantage of functions as first-class objects), but it doesn't directly address the problems. There are also parts of your code that aren't really relevant to the question, like loading content other than scripts, and there are things here that could probably use a review of their own, like using setAttribute when attribute properties would suffice... \$\endgroup\$ – Dagg Jul 20 '14 at 16:21
  • \$\begingroup\$ I guess what I'm trying to say is if you were to take the original code, explicitly point out the problems, and offer solutions (possibly based on your existing solution, but probably not identical to it), your answer might be better received here. +1 from me anyway, I see where you were going with this. \$\endgroup\$ – Dagg Jul 20 '14 at 16:21
  • \$\begingroup\$ @Dagg I changed my answer. Now it focuses on the original questions \$\endgroup\$ – maja Jul 20 '14 at 18:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.