Permalink
Browse files

Core: implement ready without Deferred

- Make jQuery.ready promise-compatible
- Gives up sync guarantee for post-ready callbacks

Fixes gh-1778
Fixes gh-1823
Close gh-2891
  • Loading branch information...
timmywil committed Jan 19, 2016
1 parent 6072d15 commit 5cbb234dd3273d8e0bbd454fb431ad639c7242c1
Showing with 210 additions and 44 deletions.
  1. +4 −0 Gruntfile.js
  2. +11 −2 build/tasks/build.js
  3. +109 −0 src/core/ready-no-deferred.js
  4. +19 −31 src/core/ready.js
  5. +67 −11 test/unit/ready.js
@@ -63,6 +63,10 @@ module.exports = function( grunt ) {
callbacks: [ "deferred" ],
css: [ "effects", "dimensions", "offset" ],
"css/showHide": [ "effects" ],
deferred: {
remove: [ "ajax", "effects", "queue", "core/ready" ],
include: [ "core/ready-no-deferred" ]
},
sizzle: [ "css/hiddenVisibleSelectors", "effects/animatedSelector" ]
}
}
@@ -168,7 +168,8 @@ module.exports = function( grunt ) {
* whether it should included or excluded
*/
excluder = function( flag ) {
var m = /^(\+|\-|)([\w\/-]+)$/.exec( flag ),
var additional,
m = /^(\+|\-|)([\w\/-]+)$/.exec( flag ),
exclude = m[ 1 ] === "-",
module = m[ 2 ];

@@ -192,8 +193,16 @@ module.exports = function( grunt ) {
}
}

additional = removeWith[ module ];

// Check removeWith list
excludeList( removeWith[ module ] );
if ( additional ) {
excludeList( additional.remove || additional );
if ( additional.include ) {
included = included.concat( additional.include );
grunt.log.writeln( "+" + additional.include );
}
}
} else {
grunt.log.error( "Module \"" + module + "\" is a minimum requirement." );
if ( module === "selector" ) {
@@ -0,0 +1,109 @@
define( [
"../core",
"../var/document"
], function( jQuery, document ) {

var readyCallbacks = [],
readyFiring = false,
whenReady = function( fn ) {
readyCallbacks.push( fn );
},
executeReady = function( fn ) {

// Prevent errors from freezing future callback execution (gh-1823)
// Not backwards-compatible as this does not execute sync
window.setTimeout( function() {
fn.call( document, jQuery );
} );
};

jQuery.fn.ready = function( fn ) {
whenReady( fn );
return this;
};

jQuery.extend( {

// Is the DOM ready to be used? Set to true once it occurs.
isReady: false,

// A counter to track how many items to wait for before
// the ready event fires. See #6781
readyWait: 1,

// Hold (or release) the ready event
holdReady: function( hold ) {
if ( hold ) {
jQuery.readyWait++;
} else {
jQuery.ready( true );
}
},

ready: function( wait ) {

// Abort if there are pending holds or we're already ready
if ( wait === true ? --jQuery.readyWait : jQuery.isReady ) {
return;
}

// Remember that the DOM is ready
jQuery.isReady = true;

// If a normal DOM Ready event fired, decrement, and wait if need be
if ( wait !== true && --jQuery.readyWait > 0 ) {
return;
}

whenReady = function( fn ) {
readyCallbacks.push( fn );

if ( !readyFiring ) {
readyFiring = true;

while ( readyCallbacks.length ) {
fn = readyCallbacks.shift();
if ( jQuery.isFunction( fn ) ) {
executeReady( fn );
}
}
readyFiring = false;
}
};

whenReady();
}
} );

// Make jQuery.ready Promise consumable (gh-1778)
jQuery.ready.then = jQuery.fn.ready;

/**
* The ready event handler and self cleanup method
*/
function completed() {
document.removeEventListener( "DOMContentLoaded", completed );
window.removeEventListener( "load", completed );
jQuery.ready();
}

// Catch cases where $(document).ready() is called
// after the browser event has already occurred.
// Support: IE9-10 only
// Older IE sometimes signals "interactive" too soon
if ( document.readyState === "complete" ||
( document.readyState !== "loading" && !document.documentElement.doScroll ) ) {

// Handle it asynchronously to allow scripts the opportunity to delay ready
window.setTimeout( jQuery.ready );

} else {

// Use the handy event callback
document.addEventListener( "DOMContentLoaded", completed );

// A fallback to window.onload, that will always work
window.addEventListener( "load", completed );
}

} );
@@ -5,12 +5,11 @@ define( [
], function( jQuery, document ) {

// The deferred used on DOM ready
var readyList;
var readyList = jQuery.Deferred();

jQuery.fn.ready = function( fn ) {

// Add the callback
jQuery.ready.promise().done( fn );
readyList.then( fn );

return this;
};
@@ -54,43 +53,32 @@ jQuery.extend( {
}
} );

/**
* The ready event handler and self cleanup method
*/
jQuery.ready.then = readyList.then;

// The ready event handler and self cleanup method
function completed() {
document.removeEventListener( "DOMContentLoaded", completed );
window.removeEventListener( "load", completed );
jQuery.ready();
}

jQuery.ready.promise = function( obj ) {
if ( !readyList ) {

readyList = jQuery.Deferred();
// Catch cases where $(document).ready() is called
// after the browser event has already occurred.
// Support: IE <=9 - 10 only
// Older IE sometimes signals "interactive" too soon
if ( document.readyState === "complete" ||
( document.readyState !== "loading" && !document.documentElement.doScroll ) ) {

// Catch cases where $(document).ready() is called
// after the browser event has already occurred.
// Support: IE <=9 - 10 only
// Older IE sometimes signals "interactive" too soon
if ( document.readyState === "complete" ||
( document.readyState !== "loading" && !document.documentElement.doScroll ) ) {
// Handle it asynchronously to allow scripts the opportunity to delay ready
window.setTimeout( jQuery.ready );

// Handle it asynchronously to allow scripts the opportunity to delay ready
window.setTimeout( jQuery.ready );
} else {

} else {
// Use the handy event callback
document.addEventListener( "DOMContentLoaded", completed );

// Use the handy event callback
document.addEventListener( "DOMContentLoaded", completed );

// A fallback to window.onload, that will always work
window.addEventListener( "load", completed );
}
}
return readyList.promise( obj );
};

// Kick off the DOM ready check even if the user does not
jQuery.ready.promise();
// A fallback to window.onload, that will always work
window.addEventListener( "load", completed );
}

} );
@@ -2,6 +2,7 @@ QUnit.module( "ready" );

( function() {
var notYetReady, noEarlyExecution,
promisified = Promise.resolve( jQuery.ready ),
order = [],
args = {};

@@ -26,13 +27,36 @@ QUnit.module( "ready" );
};
}

function throwError( num ) {

// Not a global QUnit failure
var onerror = window.onerror;
window.onerror = function() {
window.onerror = onerror;
};

throw new Error( "Ready error " + num );
}

// Bind to the ready event in every possible way.
jQuery( makeHandler( "a" ) );
jQuery( document ).ready( makeHandler( "b" ) );

// Throw in an error to ensure other callbacks are called
jQuery( function() {
throwError( 1 );
} );

// Throw two errors in a row
jQuery( function() {
throwError( 2 );
} );
jQuery.when( jQuery.ready ).done( makeHandler( "c" ) );

// Do it twice, just to be sure.
jQuery( makeHandler( "c" ) );
jQuery( document ).ready( makeHandler( "d" ) );
jQuery( makeHandler( "d" ) );
jQuery( document ).ready( makeHandler( "e" ) );
jQuery.when( jQuery.ready ).done( makeHandler( "f" ) );

noEarlyExecution = order.length === 0;

@@ -44,7 +68,7 @@ QUnit.module( "ready" );
"Handlers bound to DOM ready should not execute before DOM ready" );

// Ensure execution order.
assert.deepEqual( order, [ "a", "b", "c", "d" ],
assert.deepEqual( order, [ "a", "b", "c", "d", "e", "f" ],
"Bound DOM ready handlers should execute in on-order" );

// Ensure handler argument is correct.
@@ -55,16 +79,48 @@ QUnit.module( "ready" );

order = [];

// Now that the ready event has fired, again bind to the ready event
// in every possible way. These event handlers should execute immediately.
// Now that the ready event has fired, again bind to the ready event.
// These ready handlers should execute asynchronously.
var done = assert.async();
jQuery( makeHandler( "g" ) );
assert.equal( order.pop(), "g", "Event handler should execute immediately" );
assert.equal( args.g, jQuery, "Argument passed to fn in jQuery( fn ) should be jQuery" );

jQuery( document ).ready( makeHandler( "h" ) );
assert.equal( order.pop(), "h", "Event handler should execute immediately" );
assert.equal( args.h, jQuery,
"Argument passed to fn in jQuery(document).ready( fn ) should be jQuery" );
window.setTimeout( function() {
assert.equal( order.shift(), "g", "Event handler should execute immediately, but async" );
assert.equal( args.g, jQuery, "Argument passed to fn in jQuery( fn ) should be jQuery" );

assert.equal( order.shift(), "h", "Event handler should execute immediately, but async" );
assert.equal( args.h, jQuery,
"Argument passed to fn in jQuery(document).ready( fn ) should be jQuery" );
done();
} );
} );

QUnit.test( "Promise.resolve(jQuery.ready)", function( assert ) {
assert.expect( 2 );
var done = jQuery.map( new Array( 2 ), function() { return assert.async(); } );

promisified.then( function() {
assert.ok( jQuery.isReady, "Native promised resolved" );
done.pop()();
} );

Promise.resolve( jQuery.ready ).then( function() {
assert.ok( jQuery.isReady, "Native promised resolved" );
done.pop()();
} );
} );

QUnit.test( "Error in ready callback does not halt all future executions (gh-1823)", function( assert ) {
assert.expect( 1 );
var done = assert.async();

jQuery( function() {
throwError( 3 );
} );

jQuery( function() {
assert.ok( true, "Subsequent handler called" );
done();
} );
} );
} )();

5 comments on commit 5cbb234

@mgol

This comment has been minimized.

Copy link
Member

mgol replied Apr 4, 2016

This commit has somehow destabilized our test suite, causing random failures in the offset module: http://swarm.jquery.org/job/2587. I've reset the run for the parent commit a few times but everything passed; resetting it on this & subsequent commits sometimes fails so this commit seems to be the culprit.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin replied Apr 4, 2016

I'm guessing it's the ready-vs-load order thing somehow.

@mgol

This comment has been minimized.

Copy link
Member

mgol replied Apr 4, 2016

@dmethvin Quite probable since this is the only source change her in the default build and offset tests weren't touched in this commit...

@mgol

This comment has been minimized.

Copy link
Member

mgol replied Apr 4, 2016

This is the failing test:

jquery/test/unit/offset.js

Lines 408 to 465 in 5d20a3c

testIframe( "offset/scroll", "scroll", function( $, win, doc, assert ) {
assert.expect( 26 );
assert.equal( $( "#scroll-1" ).offset().top, 7, "jQuery('#scroll-1').offset().top" );
assert.equal( $( "#scroll-1" ).offset().left, 7, "jQuery('#scroll-1').offset().left" );
assert.equal( $( "#scroll-1-1" ).offset().top, 11, "jQuery('#scroll-1-1').offset().top" );
assert.equal( $( "#scroll-1-1" ).offset().left, 11, "jQuery('#scroll-1-1').offset().left" );
// These tests are solely for master/compat consistency
// Retrieving offset on disconnected/hidden elements is not officially
// valid input, but will return zeros for back-compat
assert.equal( $( "#hidden" ).offset().top, 0, "Hidden elements do not subtract scroll" );
assert.equal( $( "#hidden" ).offset().left, 0, "Hidden elements do not subtract scroll" );
// scroll offset tests .scrollTop/Left
assert.equal( $( "#scroll-1" ).scrollTop(), 5, "jQuery('#scroll-1').scrollTop()" );
assert.equal( $( "#scroll-1" ).scrollLeft(), 5, "jQuery('#scroll-1').scrollLeft()" );
assert.equal( $( "#scroll-1-1" ).scrollTop(), 0, "jQuery('#scroll-1-1').scrollTop()" );
assert.equal( $( "#scroll-1-1" ).scrollLeft(), 0, "jQuery('#scroll-1-1').scrollLeft()" );
// scroll method chaining
assert.equal( $( "#scroll-1" ).scrollTop( undefined ).scrollTop(), 5, ".scrollTop(undefined) is chainable (#5571)" );
assert.equal( $( "#scroll-1" ).scrollLeft( undefined ).scrollLeft(), 5, ".scrollLeft(undefined) is chainable (#5571)" );
win.name = "test";
if ( !window.supportsScroll ) {
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );
assert.ok( true, "Browser doesn't support scroll position." );
} else {
assert.equal( $( win ).scrollTop(), 1000, "jQuery(window).scrollTop()" );
assert.equal( $( win ).scrollLeft(), 1000, "jQuery(window).scrollLeft()" );
assert.equal( $( win.document ).scrollTop(), 1000, "jQuery(document).scrollTop()" );
assert.equal( $( win.document ).scrollLeft(), 1000, "jQuery(document).scrollLeft()" );
}
// test jQuery using parent window/document
// jQuery reference here is in the iframe
window.scrollTo( 0, 0 );
assert.equal( $( window ).scrollTop(), 0, "jQuery(window).scrollTop() other window" );
assert.equal( $( window ).scrollLeft(), 0, "jQuery(window).scrollLeft() other window" );
assert.equal( $( document ).scrollTop(), 0, "jQuery(window).scrollTop() other document" );
assert.equal( $( document ).scrollLeft(), 0, "jQuery(window).scrollLeft() other document" );
// Tests scrollTop/Left with empty jquery objects
assert.notEqual( $().scrollTop( 100 ), null, "jQuery().scrollTop(100) testing setter on empty jquery object" );
assert.notEqual( $().scrollLeft( 100 ), null, "jQuery().scrollLeft(100) testing setter on empty jquery object" );
assert.notEqual( $().scrollTop( null ), null, "jQuery().scrollTop(null) testing setter on empty jquery object" );
assert.notEqual( $().scrollLeft( null ), null, "jQuery().scrollLeft(null) testing setter on empty jquery object" );
assert.strictEqual( $().scrollTop(), undefined, "jQuery().scrollTop() testing getter on empty jquery object" );
assert.strictEqual( $().scrollLeft(), undefined, "jQuery().scrollLeft() testing getter on empty jquery object" );
} );
. When I run it by itself (i.e. I go to /test/?dev&hidepasse&module=offset&testId=afd141f6) it always fails for me in current Chrome.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin replied Apr 5, 2016

Yeah so the iframe runs this ready code but the testIframe() in the parent can sneak in randomly since it's checking jQuery.isReady in the iframe jQuery which will happen first.

It would seem like a better test design to always use testIframeWithCallback() so that the iframe controlled when the code ran, but I'm worried this points to a bigger problem with assumptions that others may be making as well.

Please sign in to comment.