Skip to content

Remove default Deferred callback context #3060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Remove default Deferred callback context #3060

gibson042 opened this issue Apr 15, 2016 · 3 comments

Comments

@gibson042
Copy link
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?

@dmethvin
Copy link
Member

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

@Krinkle
Copy link
Member

Krinkle commented Apr 19, 2016

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 added a commit to jquery/jquery.com that referenced this issue Apr 29, 2016
@Krinkle
Copy link
Member

Krinkle commented Nov 23, 2016

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants