Skip to content

Don't double load jQuery#97

Merged
markstory merged 4 commits intomasterfrom
no-double-jquery
Sep 30, 2013
Merged

Don't double load jQuery#97
markstory merged 4 commits intomasterfrom
no-double-jquery

Conversation

@markstory
Copy link
Copy Markdown
Member

If jQuery is on the page and has the features DebugKit needs, don't download another copy. If jQuery is absent make a blocking XHR request to fetch it and use eval LIKE A BOSS.

If jQuery is on the page and has the features debugkit needs, don't
download another copy. If jQuery is absent make a blocking XHR request
to fetch it and use eval LIKE A BOSS.
@ADmad
Copy link
Copy Markdown
Member

ADmad commented Sep 30, 2013

👍

Comment thread webroot/js/js_debug_toolbar.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we'll need a better test than my original suggestion

"1.8.0" >= "1.8.1"
false
"1.8.1" >= "1.8.1"
true
"1.9.1" >= "1.8.1"
true
"1.10.0" >= "1.8.1"
false <---
"2.0.0" >= "1.8.1"
true

@AD7six
Copy link
Copy Markdown
Member

AD7six commented Sep 30, 2013

👍

Though as commented the version compare function needs some adjusting. Do we need min and max? Seems a bit risky to only provide a minimum and therefore leave the door open to using an incompatible major version i the future.

I'm not sure of the right logic to suggest but e.g. a simple implementation could be like:

function jqueryVersionOk(version, min, max) {

    // return true if first is >= second
    function compare(first, second) {
        var i,
            firstVersion = first.split('.'),
            secondVersion = second.split('.');

        for(i = 0; i < secondVersion.length; i++) {
            if (isNaN(firstVersion[i])) {
                return false;
            }
            if (firstVersion[i] > secondVersion[i]) {
                return true;
            }
            if (firstVersion[i] < secondVersion[i]) {
                return false;
            }
        }

        return true; // exact version match to the precision of the second version
    }

    return compare(version, min) && !compare(version, max);
}

if (window.jQuery && jqueryVersionOk(jQuery.fn.jquery, '1.8', '2') {

@markstory
Copy link
Copy Markdown
Member Author

Good point about limiting the upper boundary. I'll get that fixed.

@markstory
Copy link
Copy Markdown
Member Author

I think I have the boundary conditions fixed.

@markstory
Copy link
Copy Markdown
Member Author

I think I got it this time. I apologize about all the mistakes, my brain has been a bit mushy today.

versionWithin('1.10.1', '1.8', '2.0');
true
versionWithin('2.1.0', '1.8', '2.0');
false
versionWithin('1.8.1', '1.8', '2.0');
true
versionWithin('1.7.1', '1.8', '2.0');
false

Comment thread webroot/js/js_debug_toolbar.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the minimum length of the two variables are used - this looks like it's unreachable

@AD7six
Copy link
Copy Markdown
Member

AD7six commented Sep 30, 2013

LGTM 👍

markstory added a commit that referenced this pull request Sep 30, 2013
Don't double load jQuery.

If the host page has jQuery with a compatible version loaded, don't load a second copy. Only when jQuery is missing, old, or super new will a second copy be loaded.
@markstory markstory merged commit ada01b8 into master Sep 30, 2013
@markstory markstory deleted the no-double-jquery branch September 30, 2013 17:39
@bar
Copy link
Copy Markdown
Contributor

bar commented Nov 14, 2013

I understand DebugKit needs 1.8.1 to work. But this particular merge began to cause trouble when the actual JQuery version, loaded before DebugKit, is lower than 1.8.

From master:
XMLHttpRequest.responseText ends up with JQuery version 1.8.1.,

And later jQuery.noConflict() sets JQuery global variable to 1.8.1. which makes some of the plugins previously loaded to fail.

From that line comment, it seem to be the desired intention, but I suggest using $ in DebugKit, and making that line $.noConflict(true) so that the JQuery global variable sticks with the previous version, which may be used by other code.

You can test it here:
http://jsbin.com/EVoZoroP/1/edit?html,js,output

markstory added a commit that referenced this pull request Nov 14, 2013
Not using true only restores $, however userland code will probably also
reference jQuery which will be the newer version.

Refs #97
@markstory
Copy link
Copy Markdown
Member Author

@bar Thanks for the bin, I added noConflict(true).

@bar
Copy link
Copy Markdown
Contributor

bar commented Nov 14, 2013

You are fast 👍 I was just about to issue the PR :p

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants