In IE8 & IE9 callbacks are executed before assets are loaded, then again once loaded #203

Closed
jezstephens opened this Issue Jan 3, 2013 · 29 comments

Projects

None yet
@jezstephens

I'm having trouble with my ready callbacks being called before assets have been loaded in IE, using HeadJS 0.99. This occurs even in basic usage, such as the following example.

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript" src="head.js"></script>
</head>
<body>
    <script type="text/javascript">
        head.ready(function() {
            $("#main").text('hello');
        });

        head.js('https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js');
    </script>

    <div id="main"></div>
</body>
</html>

This works OK in Chrome where the 1st version of api.load is used. In IE however, the 2nd implementation using the text/cache hack is used, and does not work.

I have seen the warning that "If caching is not configured correctly on the server, then items could load twice", but I have used HttpWatch to verify IE is caching the assets, and it is.

When api.load is called in IE, it is entering the (!isHeadReady) block, scheduling itself to be called again later, and returning without calling getAsset(). As a result, the assets array remains unpopulated.

api.load = function () {
    var args = arguments,
        rest = [].slice.call(args, 1),
        next = rest[0];

    // wait for a while. immediate execution causes some browsers to ignore caching
    if (!isHeadReady) {
        queue.push(function () {
            api.load.apply(null, args);
        });

        return api;
    }

Next, domReady() is called via domContentLoaded(), and the built-in DOM ready callback is executed. In this callback, the call to allLoaded() returns true because there are no non-loaded assets (because the assets array has not been populated), and so the user callbacks are executed before the assets have been loaded. Is it expected that domReady() can be called before the rescheduled call to api.load() in this way?

// perform this when DOM is ready
api.ready(doc, function () {

    if (allLoaded()) {
        each(handlers.ALL, function (callback) {
            one(callback);
        });
    }

Eventually the rescheduled call to api.load() executes, and the assets are loaded. After this, the user callbacks are executed a second time.

Any input much appreciated - thanks a lot.

@1stvamp
1stvamp commented Jan 8, 2013

We're having this same issue as well with 0.99 and earlier, specifically in IE9.

@leepa
leepa commented Jan 8, 2013

Hate to do the 'me too' thing - but we're suffering this problem on our site too.

@mavbozo
mavbozo commented Jan 12, 2013

Using labeled scripts solved that problem, I think.
Here is my problem in IE9:
I got this in developer console

SCRIPT5009: '$' is undefined
headjs.html, line 9 character 13

My code is:

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript" src="http://cdnjs.cloudflare.com/ajax/libs/headjs/0.99/head.min.js"></script>
</head>
<body>
    <script type="text/javascript">
        head.ready( function() {
            $("#main").text('hello');
        });

        head.js( { "jQuery" : 'https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js'} );
    </script>

    <div id="main"></div>
</body>
</html>

by using label in
head.ready( "jQuery", function() { ...
There are no errors

<!DOCTYPE html>
<html>
<head>
    <script type="text/javascript" src="http://cdnjs.cloudflare.com/ajax/libs/headjs/0.99/head.min.js"></script>
</head>
<body>
    <script type="text/javascript">
        head.ready( "jQuery", function() {
            $("#main").text('hello');
        });

        head.js( { "jQuery" : 'https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js'} );
    </script>

    <div id="main"></div>
</body>
</html>
@joseadrian

@maverickbozo you are right! It worked!

@cheesegrits

Any news on this?

This issue is absolutely killing my app in IE 9 and 10. I've confirmed that using labeled scripts "solves" the problem, but my app is large and complex, which is why we use head.js, and trying to work around this by using labeled calls everywhere we use head.ready() would be a nightmare, and basically obviate the whole reason we use head.js in the first place!

I'm happy to do anything and everything I can to assist in testing any potential fixes.

-- hugh

@1stvamp
1stvamp commented Feb 16, 2013

@cheesegrits we've had exactly the same issue, and the same reason we couldn't just use labels, for now I've had to put the following monkey patching in place for IE9 and 10:

/*
   This is a workaround for this bug in head.js:
   https://github.com/headjs/headjs/issues/203

   It works by monkeypatching head.js() calls to just insert <script></script> tags
   at the end of the document in Internet Explorer.

   As soon as head.js is patched upstream we should use that and remove this workaround.
 */

window._IE_HEAD_SCRIPTS = [];
head.js = function()
{
    var arr = Array.prototype.slice.call(arguments);
    window._IE_HEAD_SCRIPTS = window._IE_HEAD_SCRIPTS.concat(arr);
};

function insertHeadScriptsForIE()
{
    for(var i=0; i < window._IE_HEAD_SCRIPTS.length; i++)
    {
        document.write('<script type="text/javascript" src="' + window._IE_HEAD_SCRIPTS[i] + '"><\/script>\n');
    }
}

(insertHeadScriptForIE() is conditionally called at the bottom of body)

The file is conditionally loaded from our templates for IE only (actually we've found if we always make sure we use .ready there haven't been the same issues in IE10, seems waiting for head.js to fire the ready event as well works for 10, so we're just loading for IE9 and below.

@cheesegrits

It seems that the issue is more unpredictable on IE10 than in IE9, errors seem to move around from page load to page load, and improve slightly if I use the uncompressed head.js. But I've still never managed to get a completely clean page load of our app, without using labeled head.ready() calls. But that just isn't an option, as our app is a Joomla "application builder" extension, with over 120 different optional plugins, and the permutations of dependencies are just too numerous to label everything.

Thank you for the response, and I'll give it a go.

Could you possibly post your conditional call of insertHeadScriptForIE()? Just so I make darn sure I'm getting that part right.

-- hugh

@1stvamp
1stvamp commented Feb 16, 2013

@cheesegrits as we don't have issues with IE10 we use an IE<=9 conditional:

<!--[if IE]><script>insertHeadScriptsForIE();</script><![endif]-->

However if you're having issues still in 10 but you use jQuery you can use the browser detection object available up til 1.9 (and as a plugin after):

if ($.browser.msie)
{
    insertHeadScriptsForIE();
}
@cheesegrits

Thanks. We're a MooTools shop, although I'm a big jQuery fan, and find pros and cons with both. Joomla went with Moo early on, so we have about 100k lines of Moo heavy JS code. Of course, the New Blood coming in to the Joomla project are all jQuery fans, so now it's using both, which does somewhat complicate life, ::sigh::.

About to attempt to have a life, it being Saturday evening, but I'll revisit this tomorrow. Am curious as to why I'm having issues with IE10 where you aren't.

-- hugh

@itechnology
Contributor

Will have a detailed look at this.

Honestly i'm not a big fan of script loaders, especially when one is already using jQuery or other which is more than capable of injecting scripts on their own ..but i understand the need in cms's where you have to manage lot's of unpredictable plugin's ..and well, it seems something is not quite working as advertised !

For v2 i'm contemplating to moving to using commonJS/promises, as the way things are currently in HeadJS are not necessarily easily understandable.

@cheesegrits

To be honest, in the next release of our J! extension, currently in early Beta, we've switched to RequireJS, as we just found it suited our requirements better. But we'll still have two versions out there with several thousand users using head.js, and we do appreciate the work you put in.

But we really, really need to work out what's going on with the premature callbacks in IE9. IE9 adoption just seemed to get a bump in the last few months, and a lot of our users are hammering on us to get this fixed.

Once again, thanks for the effort.

@itechnology
Contributor

RequireJS for loading is probably more mature than HeadJS ..i only took over the project a while back, and from what i know most of the HeadJS code dates back to the FF3.0 era. I updated bits and pieces here and there, but even so i think some things need to be rethought.

Personally i never even use script loaders since i usually work with jQuery which is pretty capable on it's own to load and inject scripts if ever there be the need. What i do use extensively are the responsive features and browser detections. My golden motto is: css in the head, js in the bottom ..yeah i know, cms's have a bad habit of breaking this rule :)

..anyways, i will continue to keep patching up things, since it's an interesting exercise, and to allow others to continue using HeadJS

@cheesegrits

Ah, I didn't realize the project had changed hands.

My motto is "if it's free, I'll take two". But that's of limited relevance in this situation. :)

Anything I can do to help test fixes, just shout. If you want to do some joint debugging, you can find me on Skype ('cheesegrits'). I really, really need to find a reliable fix for this. For various reasons too gory to relate, the "monkeypatch" kindly provided by @1stvamp isn't going to work in our usage, without some serious surgery on our code.

@cheesegrits

OK, first off, 0.99 seems to work OK in IE10. At some point I had put my test page in IE9 mode and forgotten about it. Doh!

For IE9, I have put in a Horrible Hack in head.js itself, just to get us limping along until someone can fix the real underlying cause of api.load() firing prematurely. My assumption is that in our usage, there will always be assets. So, in allLoaded(), if the browser is ie9, and assets is empty, then we're in the first, bogus call, and should just return false. So ..

    // $$$ hugh - added for use in Horrible Hack in allLoaded
    function countProperties(obj) {
        var count = 0;

        for(var prop in obj) {
            if(obj.hasOwnProperty(prop))
                ++count;
        }

        return count;
    }

    function allLoaded(items) {
        // $$$ hugh - Horrible Hack to work round premature firing in IE9
        // https://github.com/headjs/headjs/issues/203
        if (api.browser.name === 'ie' && api.browser.version === 9 && !items && countProperties(assets) === 0) {
            return false;
        }

        items = items || assets;

        for (var name in items) {
            if (items.hasOwnProperty(name) && items[name].state !== LOADED) {
                return false;
            }
        }

        return true;
    }

Like I said, Horrible Hack, but so far it seems to be working, without us having to hack on our code at all.

-- hugh

@cheesegrits

OBTW, what minifier do you use? I can't run it through our usual project Ant build, as we use JSLint prior to packing, and the head.js source fries lint's brain. I used a random online minifier that uses UglyfyJS for now, but would rather use whatever you usually use for your build.

Oh, and so far the Horrible Hack seems to be working OK with no nasty side effects, although the next 24 hours will tell for real, as I just pushed it out to our public guthub.

Obviously not to be used if you have any usage case that doesn't actually load any assets, but should be safe otherwise.

@pollen8
pollen8 commented Feb 22, 2013

close Hugh ;) we use the YUI compressor, and JSHint for code style

@itechnology
Contributor

Currently using Google Closure Compiler with --ECMASCRIPT5_STRICT and --gctSimple options. Nothing fancy, but safe.

Also use JSHint, not JSLint ..and get 21 information messages, 0 errors, 0 warning. Don't see why JSLint should be going all crazy on your side. Maybe you should review you JSLint settings ? ..or maybe me :)

@itechnology
Contributor

Got it down to 0 errors, 0 warnings, 2 information messages ..it was mainly complaining about not using dot notation on certain objects and skipping the curly brackets on some if statements...

@cheesegrits

@pollen8 - Yeah, I had a spelling / brain fart. JSHint, not JSLint. Spelling farts are something you should be quite familiar with, LMAO! ;)

[BTW, Rob is the main author of Fabrik, Effectively my Boss. And not know for his consistently good spelling]

Yeah, I just saw a long list of complaints from JSHint being spat out in our Ant build, and it refused to compress. I didn't want to touch too much of the rest of the code right that minute, as it was 3am and I just wanted to get a working version out for our users.

If we don't get an official fix soon, I'll actually maintain my own little branch, from which I can send pull requests for things like format changes to keep JSHint happy. For now, I just copied the files from my clone of the master, and worked on the copies directly in our distro tree.

@pollen8
pollen8 commented Mar 28, 2013

to note also that this effects ie8 and thus Hugh's previous fix should be:

 // $$$ hugh - added for use in Horrible Hack in allLoaded
    function countProperties(obj) {
        var count = 0;

        for(var prop in obj) {
            if(obj.hasOwnProperty(prop))
                ++count;
        }

        return count;
    }

    function allLoaded(items) {
        // $$$ hugh - Horrible Hack to work round premature firing in IE9 or earlier
        // https://github.com/headjs/headjs/issues/203
        if (api.browser.name === 'ie' && api.browser.version <= 9 && !items && countProperties(assets) === 0) {
            return false;
        }

        items = items || assets;

        for (var name in items) {
            if (items.hasOwnProperty(name) && items[name].state !== LOADED) {
                return false;
            }
        }

        return true;
    }
@cheesegrits

Strange, I could have sworn these issues were IE9 specific, but my memory is hazy on which versions I was testing on, so it's entirely possible the same problem affected IE8. I just wanted to be as specific as possible, so as not to mess with versions that didn't exhibit the problem. Although my comment said "or earlier", so I probably just had a Senior Moment, and used === instead of <=.

Any idea what effect that has on IE7? Or do we care?

It's still a Horrible Hack, though. Would love to see a real fix for this.

@thomthom thomthom referenced this issue in thomthom/SKUI Jun 19, 2013
Closed

IE Support #38

@neonwired

Still no fix for this, is head.js even alive?

@cheesegrits

We've switched to Require JS, and just using my patched version for our legacy stuff still using head.js. Haven't seen much activity here.

@xhaggi
xhaggi commented Sep 26, 2013

I use a different fix for this issue. This fix don't need to check the browser version and is applied to load.js only.

IE 7-9 use the apiLoadHack() to preload the JS files, but if the head is not ready the loads are queued and executed after 300ms with a timer. Before the timer is executed the dom ready event is triggered and no assets exists, because they are queued for loading. Now all ready callbacks are executed and then the timer is triggered which cause in double load ready callbacks. So for IE 7-9 or other browsers which use the apiLoadHack() function, we check if head is ready before executing all callback functions.

// perform this when DOM is ready
api.ready(doc, function () {
    // execute only if head is ready (fix #203)     
    if (isHeadReady && allLoaded()) {
        each(handlers.ALL, function (callback) {
            one(callback);
        });
    }

    if (api.feature) {
        api.feature("domloaded", true);
    }
});
@robert-hoffmann
Member

Ahh, thanks for the fix. Will evaluate this and integrate into the current release if possible. IE 6/7/8/9 united don't event represent 10% market-share nowadays so don't think this could pose any serious side-effects.

@xhaggi
xhaggi commented Sep 27, 2013

@robert-hoffmann changed the condition to previously check isHeadReady before allLoaded(). so no need to interate the assets object if the head is not ready.

@tzoro
tzoro commented Nov 5, 2013

@robert-hoffmann Can we except this fix soon?

@robert-hoffmann robert-hoffmann added a commit that closed this issue Nov 5, 2013
@robert-hoffmann robert-hoffmann Fix #203 81f6783
@robert-hoffmann
Member

Got missed ! Fix is in the src/dist folders, will replicate to cdn, etc later tonight

@oosterholt

This fix is removed again in c1ac34e.
What is the status of this isue now? We still have this issue on IE9 with 'latest' 1.0.3.

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