7
\$\begingroup\$

I have written this piece of JS and CSS loading code and I would like some advice on it. Anything some of the JavaScript gurus could possibly point out would be much appreciated. The code works, but I have not done extensive testing, because I am concerned about replacing functions in this manner.

A single JavaScript file containing JQuery as well as the below code will be included on all the pages. We write all the components in house (which means no Require JS) and keep them very modular separated into their own folder with the corresponding JS and CSS. You can imagine starting to use for instance a dropown, dialog and a datepicker on one page would require us to add 6 includes and this quite frankly is annoying, because I want the dependencies to resolve automatically and using JSP includes could possibly make multiple calls to the same resources.

Below is the source to load a single datepicker lazily:

;(function($){
    //All Lazily loaded components go here
    $.fn.datepicker = function(settings){
        console.log("This should only be displayed once");
        loadCSS("/res/component/datepicker/datepicker.css");
        var elem = this;
        return loadJS("/res/component/datepicker/datepicker.js", 
                    function(){return elem.datepicker(settings)});//After Load Completion the $.fn.datepicker is replaced
                                                                  //by the proper working implementation, execute it and return it so we maintain the chain
    };
}(jQuery));

function loadCSS(absoluteUrl){
    if(loadCSS[absoluteUrl])
        return;//Css already loaded

    $('<link>')
      .appendTo('head')
      .attr({type : 'text/css', rel : 'stylesheet'})
      .attr('href', absoluteUrl);//Appending entire element doesn't load in IE, but setting the href in this manner does

    loadCSS[absoluteUrl] = true;//Memoize
}

function loadJS(absoluteUrl, onComplete){
    if(loadJS[absoluteUrl])
        return;//Script already loaded

    loadJS[absoluteUrl] = true;//Memoize

    var result;
    jQuery.ajax({
        async : false,//Synchronized because we need to maintain the JQuery chain
         type :'GET',
          url : absoluteUrl,
     dataType :'script',
      success : function(){
                    result = onComplete();
                }
    });

    return result;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You're using the term memoise wrong. Memoisation is the act of storing the result of an expensive computation for cheap retrieval later. Here, you're instead marking / flagging a given URL as having been loaded. \$\endgroup\$ – Schism Sep 13 '14 at 7:06
  • \$\begingroup\$ I'm also curious as to why you do it two separate ways. In particular, you load scripts asynchronously without a complete function, and synchronously with one. This sort of eliminates the whole point of the onComplete callback... \$\endgroup\$ – Schism Sep 13 '14 at 7:09
  • \$\begingroup\$ Thank you for the replies, the reason for loading the scripts synchronously is to maintain the JQuery chain. If you look at the way these components are generally written you will find that they return their instances and this allows you to further call methods on the result. E.g. $.datePicker().fadeIn(100).fadeOut(100).fadeIn(100).fadeOut(100); etc \$\endgroup\$ – Gerrit Brink Sep 15 '14 at 6:29
  • 3
    \$\begingroup\$ You really should Require js (or any other AMD loader). It was designed for this very purpose... \$\endgroup\$ – Pinoniq Oct 3 '14 at 12:27
2
+50
\$\begingroup\$

A common question,

from a once over;

  • Don't worry about replacing the function, that should work just fine
  • Your comment placement makes the code stretch too much horizontally, consider placing them prior to the statement you are making the comment about
  • Don't do console.log() for production code
  • Since you don't load JavaScript asynchronously, you dont need result nor success nor onComplete
  • Consider curly braces, even for one line statements

I would personally use the full require.js, but if I had to run with this, I would modify it like this:

(function($){
    //All Lazily loaded components go here
    $.fn.datepicker = function(settings){
        //After Load Completion the $.fn.datepicker is replaced
        //by the proper working implementation, execute it and return it so we maintain the chain
        loadCSS("/res/component/datepicker/datepicker.css");
        loadJS("/res/component/datepicker/datepicker.js"); 
        return this.datepicker(settings)});                                                          
    };
}(jQuery));

function loadCSS(absoluteUrl){
    //Did we load the stylesheet already?
    if(loadCSS[absoluteUrl]){
     return;
    }
    loadCSS[absoluteUrl] = true;
    //Actually load the stylesheet
    $('<link>')
      .appendTo('head')
      .attr({type : 'text/css', rel : 'stylesheet'})
      //Appending entire element doesn't load in IE, but setting the href in this manner does
      .attr('href', absoluteUrl);
}

function loadJS(absoluteUrl){
    //Did we load the the JavaScript file already?
    if(loadJS[absoluteUrl]){
        return;
    }
    loadJS[absoluteUrl] = true;
    //Synchronized because async is hard
    jQuery.ajax({
        async : false,
         type :'GET',
          url : absoluteUrl,
     dataType :'script'
    });
}
\$\endgroup\$
3
\$\begingroup\$

No JavaScript guru, but I do have some thoughts on this.


As you yourself have point out in comments, this will be a problem when loadJS is called with the same URL a 2nd time before the 1st call has completed:

function loadJS(absoluteUrl, onComplete){
    if(loadJS[absoluteUrl])
        return;//Script already loaded

    loadJS[absoluteUrl] = true;//Memoize
    // ...

As loadJS[absoluteUrl] gets set to true immediately, even before the URL is really loaded, subsequent calls to the method will return early. By moving the loadJS[absoluteUrl] = true; line further down in the method, somewhere after the onComplete() call, the problem will be mitigated. In that case, the worst thing that can happen is that the URL will be loaded twice, which is not as bad as the rest of the code happily executing thinking that initialization has successfully completed when in fact it hasn't yet.

Update: ok maybe that's not the worst thing that can happen (as you pointed out in comments). For example, if the same JS file is loaded by multiple loadJS calls in parallel, the functions in the JS file will get overwritten. I don't know how that works, and what happens if the function is being overwritten while it's also being executed at the same time. To avoid such issues, you would need to implement locking in loadJS to prevent subsequent calls while a URL is being loaded.


It seems a bad idea to use the same name for objects and functions, for loadCSS and loadJS, for many reasons:

  • Can be confusing and lead to nasty bugs (see more on that in my update)

  • loadCSS and loadJS are not good names for objects that act essentially as a cache of url -> flag, indicating whether a URL was already loaded or not

  • Since CSS and JS URLs are distinct, a single cache will be enough

So how about renaming the cache, and sharing between CSS and JS:

function loadCSS(absoluteUrl) {
    if (alreadyLoadedUrls[absoluteUrl])
        return;  // Css already loaded

    // ...
}

function loadJS(absoluteUrl, onComplete) {
    if (alreadyLoadedUrls[absoluteUrl])
        return;  // Script already loaded

    // ...
}

Another thing, loadJS returns different things when the URL was already loaded or not:

  • If already loaded, returns null
  • If called for the first time, it returns onComplete()

I think you should make the behavior consistent: either return null always, or onComplete() always.

Update

As you asked, a bit more info why not to use the same name for different objects.

As a general principle, programs should be as clear as possible. Using the same name for different objects is really just asking for trouble, for no apparent benefit, at least in this example. The handling of and behavior of this feature in JavaScript can be confusing, and may also be browser-dependent.

Take a look at the example snippets in this other answer. Predicting the output of these examples is far from trivial:

(function (foo) {
  var foo; // doesn't affect the existing `foo` identifier
  return typeof foo;
  function foo () {}
})('string'); // yields "function"

(function (foo) {
  var foo = {};
  return typeof foo;
  function foo () {}
})('string'); // yields "object"!!

Having such elements in the code that force me to think hard would be a really unnecessary obstacle.

Even though in your code there is no such confusion, that might creep in later as you (or somebody else) add more features later. It also sets a bad example that others might follow. Unless there is a really good reason to do this, and real advantages, don't call different things by the same name, don't make me think, keep things perfectly clear.

\$\endgroup\$
  • \$\begingroup\$ Can you elaborate on the bugs in your first point? I can however foresee one issue with this approach and that would be an asynchronous request made at the exact same time on a slow connection would cause it to return without completing loading. I have familiarized myself with require js and even though I would like to obtain a simpler solution it seems it does meet half my requirements and the source is quite understandable. \$\endgroup\$ – Gerrit Brink Oct 5 '14 at 7:26
  • \$\begingroup\$ The current flaw in this approach is that if 2 requests for the same js file is made asynchronously within a short time frame from each other (lets say the file load takes 5 seconds) the flag would be set for the initial URL and the subsequent request would return without the file being initialized. Further synchronization on this function is also not feasible due to it's negative impact on performance. This loading system is meant for a framework I am currently developing and I have concluded that I will rather inject external links from the back end keeping it as simple as possible. \$\endgroup\$ – Gerrit Brink Oct 5 '14 at 10:37
  • \$\begingroup\$ I see your point, and I think I have a workaround. See near the top of my post. \$\endgroup\$ – janos Oct 5 '14 at 16:53
  • \$\begingroup\$ This could be done yes, but it might be risky. Lets assume we are loading the datepicker. If the component is initialized initially it overwrites the single datepicker function which is simple function with no dependencies or sub functions. Upon initialization all the corresponding dependencies and sub functions are loaded as well. If another datepicker call is then made it could potentially overwrite the functions currently in use by the previous instance. This is however based on an assumption and would require thorough knowledge on the JS engine and or browser. \$\endgroup\$ – Gerrit Brink Oct 6 '14 at 9:17
  • \$\begingroup\$ You're right. I added another note on this. Unfortunately I don't know a good solution, other than adopting RequireJS, or borrowing from its source code. \$\endgroup\$ – janos Oct 6 '14 at 9:48
2
\$\begingroup\$

If you want to load JS resources asynchronously but worry about repetitive initialization, you could cache your onComplete callbacks:

function loadJS (url, transformer, onComplete) {
  if (isLoading(url))
    return addOnCompleteCallback(url, onComplete);
  if (isLoaded(url))
    onComplete(getResource(url));

  setLoadingFlag(url);

  ajax.load(url, function (err, res) {
    setResource(transformer(res));
    setLoadedFlag(url);

    getOnCompleteCallbacks(url)
      .forEach(function (callback) {
        callback(getResource(url));
      });
  });
}

Complete pseudo-code. I have little experience with front-end development but have played with Node.js for a while.

\$\endgroup\$

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.