head.load, if the last item is ready (state=4) and a previous item is not, the callback is never called. #262

Closed
johnhoven opened this Issue Oct 7, 2013 · 17 comments

Projects

None yet

3 participants

@johnhoven

An example is here. See console messages. Tested in Chrome 29.

http://jsfiddle.net/HsJwC/

Lines 48-60 of source below (load.js). On line 53, the callback is passed for loading the last asset. If it is already loaded, the callback wrapper is called immediately. If all items are not loaded, the callback wrapper does not call the actual callback. As far as I can tell, there is nothing in the loading of the other assets which would cause them to see they are the last script to be loaded and call the callback.

Since the one(callback) will only be executed once AND the callback wrapper checks that all are loaded, would it hurt to pass the callback as the 2nd parameter in the load call (line 53) on every iteration?

each(args, function (item, i) {
  if (item !== callback) {
    item             = getAsset(item);
    items[item.name] = item;

    load(item, callback && i === args.length - 2 ? function () {
      if (allLoaded(items)) {
         one(callback);
      }

    } : null);
  }
});
@johnhoven

Lines 273-285 in the current source. I had grabbed the source from the download link on the website. Part of function apiLoadAsync().

@johnhoven

OK, not quite that simple (suggestion above to always pass the callback). As the items array is lazily loaded, so allLoaded(items) will be true after the first item.

@robert-hoffmann
Member

Sorry for the delay.

Actually your sample does fire the callbacks if i remove the comments you inserted (Chrome 30)
http://jsfiddle.net/HsJwC/3/

However you are passing an array to HeadJS, 0.99 does not support arrays so actually in the second example jQueryUI never loads, here is the corrected example
http://jsfiddle.net/HsJwC/4/

@robert-hoffmann
Member

Ok i take that back ..just retested. The array thing was effectively breaking the load of UI, but now single + multiple ..files will load but multiple callback does not fire. Then calling multiple again, will fire the callback no matter which way files are then called ..brain scratching time

@johnhoven

setTimeout probably isn't the most elegant way around this, but it was a quick and dirty way around it. Otherwise, I think you need to set up the items array before-hand and then check if all are loaded after each script is loads/is recognized as already loaded. I don't remember why I was using an array for the fiddle, but here is a version with the below tweak, without the array usage, and also the order of the jquery/ui scripts has been fixed. http://jsfiddle.net/53aX8/

.each(args, function (item, i) {
  if (item !== callback) {
    item             = getAsset(item);
    items[item.name] = item;

    load(item, callback && i === args.length - 2 ? function () {

    // START REVISIONS - create a delegate which won't fire callback until all are loaded
    var callDelegateWhenAllLoaded = function () {
        if (allLoaded(items)) {
            one(callback);
        }
        else{
            window.setTimeout(callDelegateWhenAllLoaded, 10);
        }
     };
     // Initiate
     callDelegateWhenAllLoaded();
     // END REVISIONS

    } : null);
  }
});
@robert-hoffmann
Member

This seems todo the trick

.each(args, function (item, i) {
  if (item !== callback) {
    item             = getAsset(item);
    items[item.name] = item;

    load(item, function () {
        if (allLoaded(items)) {
            one(callback);
        }
    });
  }
});

Needs more research to see if it doesn't break anything, but i don't think so.

Thing is if you would have inverted jquery.min & jqueryui.min in your first code samples, the callback works, and this would have slipped by.

..i really need to take some time to weed out a bunch of old code that adds more confusion than usefulness

@robert-hoffmann
Member

btw if this does the trick, i'll slip in the array loading from 1.0-wip in the update since it's retro-compatible anyways ;-)

@johnhoven

That works for the original issue (where the last script had already been loaded) but fails if the first script has already been loaded. The callback can be executed immediately if the first item is loaded. (http://jsfiddle.net/XV72X/)

How about this: (http://jsfiddle.net/eE2gy/)

   if (!isFunction(callback)) {
       callback = null;
   }

    // JRH #262 Revision.  First populate the items array.
    // When allLoaded is called, all items will be populated.
    // Issue when lazy loaded, the callback can execute early.
    each(args, function (item, i) {
        if (item !== callback) {                
            item             = getAsset(item);
            items[item.name] = item;
        }
    });

    each(args, function (item, i) {
        if (item !== callback) {                
            item = getAsset(item);
            // Revised                
            load(item, function () {
                if (allLoaded(items)) {
                    one(callback);
                }
            });
            // End Revised
            // Start Pre Revision
            //load(item, callback && i === args.length - 2 ? function () {
              //  if (allLoaded(items)) {
              //      one(callback);
              //  }
            //} : null);
            // End pre Revision
        }
    });
@robert-hoffmann
Member

I'll dig into this some more tomorrow, but for sure the callback should only fire once (after) all scripts have been checked/validated as not loaded, or already loaded, not matter in which way the are supplied

@robert-hoffmann
Member

Are you sure, it seems to work fine for me, unless i missed something. Check this page out: http://headjs.com/test/load.html

I tested this under FF/Chrome
jQuery -> jQuery/jQueryUI
jQuery -> jQueryUI/jQuery

jQuery/jQueryUI -> jQuery
jQueryUI/jQuery -> jQuery

Does this work for you ?

@johnhoven

Pretty sure. The issue isn't that the callback doesn't get called after your change, it is that it gets called before all of the scripts are loaded. Since the test page isn't actually using the scripts, it is hard to notice.

Modify the "Load jQuery/jQueryUI" handler try to use jQuery UI. It will be executed before jquery UI is loaded because the handler is executed immediately after the script identifies that jQuery is already loaded

when you execute like so:

  1. "Load jQuery". (so that jQuery is already loaded).
  2. "Load jQuery/jQueryUI".

you'll get the exception using jQueryUI.

Or use this one: http://jsfiddle.net/Vnwm3/

@robert-hoffmann
Member

ok, so actually we had 2 errors, one where the callback could downright be broken (what my use case fixes), and that the callback can fire before a resource has finished loading (what you where actually talking about)

i got it right this time ?

@johnhoven

Right, your potential fix (#262 (comment)) introduced the second bug (because before, you only fired the callback after the last asset).

I believe (#262 (comment)) addresses the second bug by pre-loading the assets array.

@robert-hoffmann
Member

Agree, this #262 (comment) seems to do the trick just right.

Updated test page http://headjs.com/test/load.html

Also added some undocumented features which don't break backwards compatibility: array loading .load(["file1", "file2"]) / .load([{ label1: "file1" }, { label2: "file2" }]) and waiting on multiple labels .ready(["label1", "label2"])

@johnhoven

Yep, that looks great. Thanks for your help Robert.

@johnhoven johnhoven closed this Oct 17, 2013
@robert-hoffmann
Member

No problem, and thank's for your help too.

Only a single function missing now till 1.0 release is ready, which enables this (code is already done, but needs some more testing)

var items = [
    { label1: "file1.js" },
    {
        label2: "file2.js",
        options: {
            async   : true/false,
            callback: specificCallBack
        }
    }
];

head.load(items, finalCallBack);

This allows you to make "certain" files async, which can be useful when there are no dependencies on those specific files while keeping the rest sync

@jaypea
jaypea commented Jun 18, 2014

@robert-hoffmann is this 'options' thingy available somewhere? I could not find it in 1.0.3 nor 2.0.0 and kind of need it to enable media attributes in loaded CSS files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment