Remove default Deferred callback context #3060

Closed
gibson042 opened this Issue Apr 15, 2016 · 3 comments

Projects

None yet

3 participants

@gibson042
Member

I ran into an interesting obstacle when working on #3029: our longstanding behavior of using a default context for jQuery.Deferred callbacks (specifically, the deferred's base promise). It's not necessarily a blocker, but it does make $.when( $.Deferred().resolve() ).done( whatsMyContext ) very counterintuitive (in that I'm not even sure what to code for #3029). For sanity in that case—and for increased similarity with native Promises—I'd like to remove the default, making *With methods required to provide a meaningful context. In other words, the preceding code would have either undefined or global object context, depending on whether or not it was executed in strict mode.

Any objections?

@gibson042 gibson042 self-assigned this Apr 15, 2016
@gibson042 gibson042 added this to the 3.0.0 milestone Apr 15, 2016
@gibson042 gibson042 added a commit to gibson042/jquery that referenced this issue Apr 15, 2016
@gibson042 gibson042 Deferred: Remove default callback context
Fixes gh-3060
af85a48
@dmethvin
Member

Reading through the issues you filed I agree, explicit *With seems like a reasonable requirement if you really want context.

@Krinkle
Member
Krinkle commented Apr 19, 2016 edited

For most code I can't imagine any useful purpose in having the promise available as context. One would always chain on the promise directly, create a new deferred, or involve other code that produces a new promise. One wouldn't call then() inside the then-callback of the same promise.

However, there is be one jQuery-specific use case that applies to inside callbacks: state().

$.Deferred().reject().always(function () {
  console.log(this.state());
});

I don't expect this in more advanced projects, but I wouldn't be surprised to see in simple projects.

/** @return {jQuery.Promise} */
function foo() { /* .. */ }

showSpinner();
foo().always(function () {
  hideSpinner();
  if (this.state() == 'resolve') {
    // Stuff...
  }
});

It seems awkward to have to store the promise in a variable for that.

@gibson042 gibson042 added a commit that closed this issue Apr 23, 2016
@gibson042 gibson042 Deferred: Remove default callback context
Employs strict mode to simplify Deferred callback context handling.

Fixes gh-3060
Closes gh-3061
7608437
@gibson042 gibson042 closed this in 7608437 Apr 23, 2016
@gibson042 gibson042 added a commit to jquery/jquery.com that referenced this issue Apr 29, 2016
@gibson042 gibson042 Docs: Describe Deferred default callback context changes 46394ea
@Krinkle
Member
Krinkle commented Nov 23, 2016 edited

The following use case was rather counter-intuitive to migrate when using only the text on https://api.jquery.com/jQuery.Deferred/ and https://jquery.com/upgrade-guide/3.0/

function postWithToken(type, params) {
  return api.getToken(type).then(function (token) {
    params.token = token;
    return api.post(params).then(null, function fail() {
      if ( code === 'expired' ) {
        return retryOnce(type, params);
      }
      // Let caller handle any other error
      return this;
    });
}

Initial attempt:

-    return api.post(params).then(null, function fail() {
+    return api.post(params).then(null, function fail(err) {
       // Let caller handle any other error
-      return this;
+      return err;

I tried this because I previously believed that a then() kept the current state if a non-promise is returned (plain value from resolved then is resolved, plain value from rejected then is rejected) - and that creating an explicit Deferred is only needed to change the type from resolved to rejected. However for this case, the following fix is needed:

-    return api.post(params).then(null, function fail() {
+    return api.post(params).then(null, function fail(err) {
       // Let caller handle any other error
-      return this;
+      return $.Deferred.reject(err);

This works but recording it here in case it helps someone else, and in case documentation can be improved.

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