Permalink
Browse files

Deferred: Remove default callback context

Employs strict mode to simplify Deferred callback context handling.

Fixes gh-3060
Closes gh-3061
1 parent 8442759 commit 76084372c29a59b3fa790ea4d2687f0767514999 @gibson042 gibson042 committed Apr 15, 2016
Showing with 57 additions and 38 deletions.
  1. +3 −0 build/tasks/build.js
  2. +7 −7 src/deferred.js
  3. +5 −5 src/wrapper.js
  4. +42 −26 test/unit/deferred.js
@@ -23,6 +23,9 @@ module.exports = function( grunt ) {
baseUrl: "src",
name: "jquery",
+ // Allow strict mode
+ useStrict: true,
@mgol
mgol Apr 23, 2016 Member

We're now getting extra "use strict" pragmas in the middle of the built file; perhaps this could be solved in a different way?

@gibson042
gibson042 Apr 23, 2016 edited Member

I'm not sure of the inner workings, but we could always strip the directives ourselves in the build. Also note that they're already removed from the minified file, so this isn't really a big deal AFAICT.

@mgol
mgol Apr 23, 2016 Member

Yeah, it's mostly about reducing the WTF effect in people reading the source of our built file.

@markelog
markelog Apr 24, 2016 edited Member

That, doesn't sound ideal at all - introducing a hack, to fix another :/

@mgol
mgol Apr 24, 2016 Member

This is all about a harmless issue that's related to our hacked up build process; I don't see it as a huge problem and definitely as not something that should influence our decision about going strict in any way.

@markelog
markelog Apr 24, 2016 edited Member

The more "hacks" and "inventions" we introduce in the build process or otherwise, the more bad it gets in my perspective

@mgol
mgol Apr 24, 2016 Member

From my point of view not using strict mode is worse than having to apply small modifications here & there.

@markelog
markelog Apr 24, 2016 edited Member

One might understand why hacks are bad, why using strict code in this context is good, on the other hand, is unclear.

@mgol
mgol Apr 24, 2016 Member

We've already discussed it in #3061, also, we're planning to use ES6 modules in the future and then strict mode is mandatory, unless we run modules in strict mode and the full built file in sloppy mode but I think it's bad to mix it like that.

@markelog
markelog Apr 24, 2016 Member

We've already discussed it in #3061

Yes, we decided to try it.

we're planning to use ES6 modules

If and when

@mgol
mgol Apr 25, 2016 edited Member

An issue about stripping these pragmas: #3077.

+
// We have multiple minify steps
optimize: "none",
View
@@ -4,6 +4,8 @@ define( [
"./callbacks"
], function( jQuery, slice ) {
+"use strict";
+
function Identity( v ) {
return v;
}
@@ -60,7 +62,7 @@ jQuery.extend( {
.fail( newDefer.reject );
} else {
newDefer[ tuple[ 0 ] + "With" ](
- this === promise ? newDefer.promise() : this,
+ this,
fn ? [ returned ] : arguments
);
}
@@ -73,7 +75,7 @@ jQuery.extend( {
var maxDepth = 0;
function resolve( depth, deferred, handler, special ) {
return function() {
- var that = this === promise ? undefined : this,
+ var that = this,
args = arguments,
mightThrow = function() {
var returned, then;
@@ -144,8 +146,7 @@ jQuery.extend( {
// Process the value(s)
// Default process is resolve
- ( special || deferred.resolveWith )(
- that || deferred.promise(), args );
+ ( special || deferred.resolveWith )( that, args );
}
},
@@ -174,8 +175,7 @@ jQuery.extend( {
args = [ e ];
}
- deferred.rejectWith( that || deferred.promise(),
- args );
+ deferred.rejectWith( that, args );
}
}
};
@@ -282,7 +282,7 @@ jQuery.extend( {
// deferred.resolve = function() { deferred.resolveWith(...) }
// deferred.reject = function() { deferred.rejectWith(...) }
deferred[ tuple[ 0 ] ] = function() {
- deferred[ tuple[ 0 ] + "With" ]( this === deferred ? promise : this, arguments );
+ deferred[ tuple[ 0 ] + "With" ]( this === deferred ? undefined : this, arguments );
return this;
};
View
@@ -38,11 +38,11 @@
// Pass this if window is not defined yet
}( typeof window !== "undefined" ? window : this, function( window, noGlobal ) {
-// Support: Firefox 18+
-// Can't be in strict mode, several libs including ASP.NET trace
-// the stack via arguments.caller.callee and Firefox dies if
-// you try to trace through "use strict" call chains. (#13335)
-//"use strict";
+// Edge <= 12 - 13+, Firefox <=18 - 45+, IE 10 - 11, Safari 5.1 - 9+, iOS 6 - 9.1
+// throw exceptions when non-strict code (e.g., ASP.NET 4.5) accesses strict mode
+// arguments.callee.caller (trac-13335). But as of jQuery 3.0 (2016), strict mode should be common
+// enough that all such attempts are guarded in a try block.
+"use strict";
// @CODE
// build.js inserts compiled jQuery here
View
@@ -16,20 +16,21 @@ jQuery.each( [ "", " - new operator" ], function( _, withNew ) {
assert.ok( jQuery.isFunction( defer.pipe ), "defer.pipe is a function" );
- createDeferred().resolve().done( function() {
+ defer.resolve().done( function() {
assert.ok( true, "Success on resolve" );
- assert.strictEqual( this.state(), "resolved", "Deferred is resolved (state)" );
+ assert.strictEqual( defer.state(), "resolved", "Deferred is resolved (state)" );
} ).fail( function() {
assert.ok( false, "Error on resolve" );
} ).always( function() {
assert.ok( true, "Always callback on resolve" );
} );
- createDeferred().reject().done( function() {
+ defer = createDeferred();
+ defer.reject().done( function() {
assert.ok( false, "Success on reject" );
} ).fail( function() {
assert.ok( true, "Error on reject" );
- assert.strictEqual( this.state(), "rejected", "Deferred is rejected (state)" );
+ assert.strictEqual( defer.state(), "rejected", "Deferred is rejected (state)" );
} ).always( function() {
assert.ok( true, "Always callback on reject" );
} );
@@ -405,21 +406,31 @@ QUnit.test( "[PIPE ONLY] jQuery.Deferred.pipe - deferred (progress)", function(
QUnit.test( "jQuery.Deferred.then - context", function( assert ) {
- assert.expect( 7 );
+ assert.expect( 11 );
var defer, piped, defer2, piped2,
- context = {},
- done = jQuery.map( new Array( 4 ), function() { return assert.async(); } );
+ context = { custom: true },
+ done = jQuery.map( new Array( 5 ), function() { return assert.async(); } );
jQuery.Deferred().resolveWith( context, [ 2 ] ).then( function( value ) {
+ assert.strictEqual( this, context, "custom context received by .then handler" );
return value * 3;
} ).done( function( value ) {
- assert.notStrictEqual( this, context, "custom context not propagated through .then" );
+ assert.notStrictEqual( this, context,
+ "custom context not propagated through .then handler" );
assert.strictEqual( value, 6, "proper value received" );
done.pop().call();
} );
+ jQuery.Deferred().resolveWith( context, [ 2 ] ).then().done( function( value ) {
+ assert.strictEqual( this, context,
+ "custom context propagated through .then without handler" );
+ assert.strictEqual( value, 2, "proper value received" );
+ done.pop().call();
+ } );
+
jQuery.Deferred().resolve().then( function() {
+ assert.strictEqual( this, window, "default context in .then handler" );
return jQuery.Deferred().resolveWith( context );
} ).done( function() {
assert.strictEqual( this, context,
@@ -435,8 +446,7 @@ QUnit.test( "jQuery.Deferred.then - context", function( assert ) {
defer.resolve( 2 );
piped.done( function( value ) {
- assert.strictEqual( this, piped,
- "default context gets updated to latest promise in the chain" );
+ assert.strictEqual( this, window, ".then handler does not introduce context" );
assert.strictEqual( value, 6, "proper value received" );
done.pop().call();
} );
@@ -447,30 +457,39 @@ QUnit.test( "jQuery.Deferred.then - context", function( assert ) {
defer2.resolve( 2 );
piped2.done( function( value ) {
- assert.strictEqual( this, piped2,
- "default context updated to latest promise in the chain (without passing function)" );
+ assert.strictEqual( this, window, ".then without handler does not introduce context" );
assert.strictEqual( value, 2, "proper value received (without passing function)" );
done.pop().call();
} );
} );
QUnit.test( "[PIPE ONLY] jQuery.Deferred.pipe - context", function( assert ) {
- assert.expect( 7 );
+ assert.expect( 11 );
var defer, piped, defer2, piped2,
- context = {},
- done = jQuery.map( new Array( 4 ), function() { return assert.async(); } );
+ context = { custom: true },
+ done = jQuery.map( new Array( 5 ), function() { return assert.async(); } );
jQuery.Deferred().resolveWith( context, [ 2 ] ).pipe( function( value ) {
+ assert.strictEqual( this, context, "custom context received by .pipe handler" );
return value * 3;
} ).done( function( value ) {
- assert.strictEqual( this, context, "[PIPE ONLY] custom context correctly propagated" );
+ assert.strictEqual( this, context,
+ "[PIPE ONLY] custom context propagated through .pipe handler" );
assert.strictEqual( value, 6, "proper value received" );
done.pop().call();
} );
+ jQuery.Deferred().resolveWith( context, [ 2 ] ).pipe().done( function( value ) {
+ assert.strictEqual( this, context,
+ "[PIPE ONLY] custom context propagated through .pipe without handler" );
+ assert.strictEqual( value, 2, "proper value received" );
+ done.pop().call();
+ } );
+
jQuery.Deferred().resolve().pipe( function() {
+ assert.strictEqual( this, window, "default context in .pipe handler" );
return jQuery.Deferred().resolveWith( context );
} ).done( function() {
assert.strictEqual( this, context,
@@ -486,8 +505,7 @@ QUnit.test( "[PIPE ONLY] jQuery.Deferred.pipe - context", function( assert ) {
defer.resolve( 2 );
piped.done( function( value ) {
- assert.strictEqual( this, piped,
- "default context gets updated to latest promise in the chain" );
+ assert.strictEqual( this, window, ".pipe handler does not introduce context" );
assert.strictEqual( value, 6, "proper value received" );
done.pop().call();
} );
@@ -498,8 +516,7 @@ QUnit.test( "[PIPE ONLY] jQuery.Deferred.pipe - context", function( assert ) {
defer2.resolve( 2 );
piped2.done( function( value ) {
- assert.strictEqual( this, piped2,
- "default context updated to latest promise in the chain (without passing function)" );
+ assert.strictEqual( this, window, ".pipe without handler does not introduce context" );
assert.strictEqual( value, 2, "proper value received (without passing function)" );
done.pop().call();
} );
@@ -825,7 +842,8 @@ QUnit.test( "jQuery.when - joined", function( assert ) {
eventuallyRejected: true,
rejectedStandardPromise: true
},
- counter = 49;
+ counter = 49,
+ expectedContext = (function() { "use strict"; return this; })();
QUnit.stop();
@@ -840,15 +858,13 @@ QUnit.test( "jQuery.when - joined", function( assert ) {
var shouldResolve = willSucceed[ id1 ] && willSucceed[ id2 ],
shouldError = willError[ id1 ] || willError[ id2 ],
expected = shouldResolve ? [ 1, 1 ] : [ 0, undefined ],
- code = "jQuery.when( " + id1 + ", " + id2 + " )",
- context1 = defer1 && jQuery.isFunction( defer1.promise ) ? defer1.promise() : window,
- context2 = defer2 && jQuery.isFunction( defer2.promise ) ? defer2.promise() : window;
+ code = "jQuery.when( " + id1 + ", " + id2 + " )";
jQuery.when( defer1, defer2 ).done( function( a, b ) {
if ( shouldResolve ) {
assert.deepEqual( [ a, b ], expected, code + " => resolve" );
- assert.strictEqual( this[ 0 ], context1, code + " => first context OK" );
- assert.strictEqual( this[ 1 ], context2, code + " => second context OK" );
+ assert.strictEqual( this[ 0 ], expectedContext, code + " => context[0] OK" );
+ assert.strictEqual( this[ 1 ], expectedContext, code + " => context[1] OK" );
} else {
assert.ok( false, code + " => resolve" );
}

1 comment on commit 7608437

@mgol
Member
mgol commented on 7608437 Apr 27, 2016

See #3082.

Please sign in to comment.