Bug Tracker

Opened 5 years ago

Closed 5 years ago

#14483 closed bug (notabug)

the promise of a promise is not the promise

Reported by: fastfasterfastest Owned by: fastfasterfastest
Priority: low Milestone: None
Component: deferred Version: 1.10.2
Keywords: Cc:
Blocked by: Blocking:

Description

The promise method of a promise should return the promise itself.

If a target is provided then deferred.promise(target) will attach the Promise aspect onto the target and return the target as the promise. However, the promise method of that promise (the target object) does not return itself; instead it currently returns an "internal implementation object."

var obj = { };

alert( $.Deferred().promise( obj ).promise() === obj ); //should be true

This can also be observed in the done, fail and notify callbacks - normally such callbacks are called with the promise as the context. In the above case, when one has attached the promise of a deferred to an existing object, the done, fail and notify callbacks are called with the "internal implementation object" as a context, not the existing object (which is the promise) as expected.

var obj = { },
   deferred = $.Deferred();

   //make obj the promise
   deferred.promise( obj );

   obj.done(function(){
      //context should be obj
      alert( this === obj );  //should be true
   });

   deferred.resolve();

Here is a fiddle demonstrating the issues: http://jsfiddle.net/fastfasterfastest/wKZrM/

Change History (15)

comment:1 Changed 5 years ago by timmywil

Component: unfileddeferred
Owner: set to fastfasterfastest
Status: newpending

Thanks for opening a ticket!

There are several reasons for the current behavior, not least of which is limiting method access to the promise's methods. As the docs explain, the user should maintain a reference to the deferred in order resolve/reject it later; it dissociates the deferred from its promise, which is good.

Please explain why someone might expect the behavior you've described. Use cases would be helpful.

comment:2 Changed 5 years ago by timmywil

Also, I didn't make this clear, but it is better not to think of the deferred.promise() method as "making an object a promise" so much as adding promise behavior to an object. When you see it that way, I think you may understand why your illustrated conditions turn out to be false.

comment:3 Changed 5 years ago by fastfasterfastest

Status: pendingnew

I don't see how the fact that the methods of a promise is a subset of its deferred has anything to do with the issue I am reporting. Similarly, that the creator of a deferred typically holds on to the deferred so it can be resolved/rejected later, I don't think has anything to do with this. Can you please explain how this comes into play here.

I think one might expect the behavior I describe for a couple of reasons - one is that is how "regular" promises work, another is the current documentation states that deferred.promise returns a deferred's promise object while returning the target object, and yet another is comments made by jaubourg (see https://github.com/jquery/api.jquery.com/issues/375).

Either passing a target object to deferred.promise makes the target object a promise or not.

Currently, deferred.promise is documented as "return a Deferred's Promise object" and it returns the target object. That seems to imply that the target object is "made into a promise."

If the intention is NOT to make the target object a promise but merely adding some promise-ish behavior to the target object, then a couple of things follow. It means that to get to the real promise of the target object one must call the target object's promise method. If one must call the target object's promise method to get to the promise, then there is no need to extend the target object with any of the "other promise methods" (done, fail, etc.) - it is sufficient to extend the target object with a promise method (since one must call it anyway to get to the promise.) Furtermore, there is currently then a bug in deferred.promise when a target object is passed. It currently returns the target object, but it should then instead return the target object's promise (i.e., the implementation should call promise() after extending the target object before returning.)

I think it comes down to having an in implementation that is consistent and can be documented.

Given the current implementation, behavior and documentation I assumed that calling deferred.promise indeed returns a promise and passing a target object makes that object a promise and returns it. I came across this bug while starting to try to address some documentation bugs and omissions about deferred and promises, e.g. see https://github.com/jquery/api.jquery.com/issues/375

comment:4 Changed 5 years ago by timmywil

Status: newpending

I don't see why we can't have the cake. Wouldn't it suffice to simply change the description? Perhaps,

"Return a Deferred's Promise object or the target object when affixing promise behavior."

I think the rest of the docs expound on this behavior clearly:

If target is provided, deferred.promise() will attach the methods onto it and then return this object rather than create a new one. This can be useful to attach the Promise behavior to an object that already exists.

Regardless, you have not actually provided a use case.

comment:5 Changed 5 years ago by fastfasterfastest

Status: pendingnew

Yes, one way to fix a bug is to change the documentation so it corresponds with the behavior. Another, is to change the behavior so it corresponds with the documentation, and if documentation is lacking or missing, with the intended behavior.

I have interpreted the intended behavior from existing behavior and from (somewhat lacking) documentation and from jaubourg's comments. I think it makes sense for deferred.promise, when called with a target object, make the target object a promise.

If the intention is NOT to make the target object a promise, then why is the target object extended with methods other than the promise method, e.g. done, fail, etc.? The fact that the target object is extended with those methods is further evidence that the intention is to make the target object a promise. Perhaps/Hopefully jaubourg will chime in if he sees this ticket.

Even if the intention is NOT to make the target object a promise but merely adding some promise-ish behavior to the target object, then as stated I think deferred.promise should still return a promise (it is simple enough, all it has to do is call promise() when it is returning), and there is no need to change the documentation.

Why do you think it is better not to return a promise from a method that at all other times returns a promise, and is also named promise()?

With regard to use case - I had created minimal test cases to demonstrate the bug. I thought that would be sufficient. I have also mentioned consistency, which I think is important and a good thing and simplifies understanding for users. "Regular" promises, those created and returned by calling deferred.promise() without passing an existing target object behaves in a certain way. It would be very good, for consistency and simplicity, to have promises that are created and returned by calling deferred.promise(obj) and passing an existing target object behave the same way. And I have pointed out two ways they do not and provided simple test cases for them.

I don't know how sophisticated of a use case you are asking for, but here is a simple one: An existing object is set as promise. When the associated deferred is resolved, any done callbacks are called with the promise/existing object as the context.

// Existing object
var obj = {
    hello: function( name ) {
      alert( "Hello " + name );
    }
  },
  // Create a Deferred
  defer = $.Deferred();

// Set object as a promise
defer.promise( obj );

// Resolve the deferred
defer.resolve( "John" );

// Use the object as a Promise
obj.done(function( name ) {
  //currently break; context is not obj
  this.hello( name ); // Should alert "Hello John"
});

comment:6 Changed 5 years ago by gibson042

I think this is a legitimate bug that we should fix.

comment:7 Changed 5 years ago by jaubourg

This is working exactly as intended.

The use case here is to add a method to the Promise object. Amazingly, you can achieve that by adding a method to the Promise object:

defer.promise().hello = function( name ) {
  alert( "Hello " + name );
};

defer.resolve( "John" );

defer.done(function( name ) {
  this.hello( name ); // Alerts "Hello John"
});

Even if you wanted to do something more complex (like when you have a prototyped approach), you can use resolveWith to control which context is used:

function MyClass() {
}

MyClass.prototype = {
  hello: function( name ) {
    alert( "Hello " + name );
  }
};

defer.resolveWith( defer.promise( new MyClass() ), "John" );

defer.done(function( name ) {
  this.hello( name ); // Alerts "Hello John"
});

Keep in mind that the internal Promise object associated to a Deferred has to be the same object reference for the entire lifetime of the Deferred. The Promise is a public object that will be shared throughout your application, it has to always be the same reference or else you wouldn't be ensured that what you add on the Promise, method or attribute alike, would still be there at the time of resolution: another third-party could very well replace the Promise with another object.

Of course, the best, free of side-effects, approach is to create a separate object and set it as the context manually using resolveWith like shown above. But you still have use for additional data on the Promise object is not just for debugging purpose. It also doesn't kill that you can compare two promises using strict equality and know if they represent the same Task or not.

As to why the promise( object ) returns the object and not the promise, it's so that litterals can be used. It is sane chaining that avoids dummy variable declarations:

// Current Behaviour
function addHello( defer ) {
  return defer.promise( {
    hello: function( name ) {
      alert( "Hello " + name );
    }
  } );
}

// Returning the promise instead
function addHello( defer ) {
  var obj = {
    hello: function( name ) {
      alert( "Hello " + name );
    }
  };
  defer.promise( obj );
  return obj;
}

So timmywil is right, we should ammend the documentation.

comment:8 Changed 5 years ago by fastfasterfastest

Ok. So to summarize, this means that in the jquery deferred implementation a promise is always the "internal implementation object". A user cannot create or use his/her own object as a promise; the only way to get your hands on a promise object is to call deferred.promise() without a parameter.

So, we have three kinds of objects:

  1. deferred object
    A deferred object is created and returned by $.Deferred().
  2. promise object
    A promise object is returned by deferred.promise(), and promise.promise() - note, passing no parameters.
  3. an object that has a promise method that returns a promise object
    An object that has a promise method that returns a promise object is typically a plain javascript object, created by the user, and then passed to deferred.promise(obj). Such objects are not promise objects.

The documentation certainly needs to be updated to reflect this. There are several places where it is either explicitly or implicitly stated that one can make an existing object a promise by calling deferred.promise(obj).

Also, the documentation in several places currently states that objects should be a deferred or a promise, when in fact they should be "an object that has a promise method that returns a promise object." It would be great if there is a simple, usable term that could be used in the documentation for such objects, objects that has a promise method that returns a promise object. Do you have any suggestion?

Jaouburg - why does deferred.promise(obj) extend the target object with methods other than promise(), e.g. done, fail, etc.? I am not sure if it can be changed at this point for backward compatible reasons, but I think that is confusing and gives users the impression that the obj has become a promise object, when in fact it has not. It attaches the Promise methods to an object, but that object is still not a promise. It merely has turned an object to "an object that has a promise method that returns a promise object".

another third-party could very well replace the Promise with another object

And you will have the same problem if you add methods to the system provided promise object, as in your first example. A third-party could very well replace your methods, or you theirs.

comment:9 Changed 5 years ago by jaubourg

I think you're confusing THE promise associated with a Deferred instance with the notion of A promise.

Any object that implements the Promise interface is a Promise. It doesn't mean that it is THE promise associated to a specific Deferred instance.

Also...

And you will have the same problem if you add methods to the system provided promise object, as in your first example. A third-party could very well replace your methods, or you theirs.

There's a difference between some method/property name collision and the border effect of replacing an internal object.

comment:10 Changed 5 years ago by fastfasterfastest

Any object that implements the Promise interface is a Promise.

Yes, but it is not sufficient to implement the methods of the Promise interface to qualify an object as a Promise - the methods also have to be implemented "correctly" according to the spec.

It has been stated:

  1. the promise of a promise is the promise itself
  2. calling deferred.promise(target) attaches the promise aspect to target
  3. any object implementing the Promise interface is a Promise

So, after calling deferred.promise(target) one can deduce the following:

  1. target implements the Promise interface (follows from 2)
  2. target is A promise (follows from 3)

Thus, calling target.promise() should then return target itself (follows from 1). But it does not. So, something is not right.

Either 1, 2 and/or 3 is not correct, or the implementation is not correct, or a combination thereof.

There are issues in the current documentation compared to the current behavior and implementation. I was going to try to correct the documentation and while doing so I came across this bug. But if this turns out this is not a bug, it will be hard to correct the document in a straight-forward, clear and simple way - there are subtleties and inconsistencies in the current behavior and implementation.

I think the source of the issues might be the deferred.promise(target) and its attempt to do "too much", trying to make target a promise.

Perhaps deferred.promise shouldn't take any parameter. Instead, there could be a deferred.observable(target) method which would make target "an observable through a promise" by adding only one method to target, .promise, which would return THE promise. Note, it would not attach the whole promise aspect, only the "observable through a promise" aspect (a single .promise method). Such a change would make it more explicit of the three kinds of objects involved.

Another idea might be that if you want to attach the promise aspect onto a target object you must supply the target when the deferred is created. I.e., $.Deferred() could take an optional parameter, a target object, and if supplied the promise interface will be attached to target and target will be THE Promise of the deferred. If not supplied, $.Deferred creates an object to use as promise as today. deferred.promise() would then no longer take any parameter.

comment:11 Changed 5 years ago by fastfasterfastest

If the implementation of promise's promise method changed from

promise: function( obj ) {
   return obj != null ? jQuery.extend( obj, promise ) : promise;
}

to

promise: function( obj ) {
   return obj != null ? jQuery.extend( obj, promise ) : (this === deferred ? promise : this);
}

then I think the bug I reported would be fixed. If a target is provided to deferred.promise() then that target would be made into a promise - it would implement the promise interface and it would return itself when calling its promise method.

The done, fail, notify callbacks of target, though, would still be called with the deferred's promise as the context, not the target itself. Although unfortunate in my opinion, that can be clearly documented.

comment:12 Changed 5 years ago by fastfasterfastest

@Jaubourg - on a somewhat related topic, the implementation of deferred's resolve, reject and notify methods (line 3305 in http://code.jquery.com/jquery-git1.js):

deferred[ tuple[0] + "With" ]( this === deferred ? promise : this, arguments );

I don't know all the intricacies of the implementation, but can the context ever be anything other than deferred? If not, that could be simplified to:

deferred[ tuple[0] + "With" ]( promise, arguments );

And, even if the context can be something other than the deferred, shouldn't the callbacks be fired with the deferred's promise as the context anyway? Unfortunately I am not set up to run the unit tests here, so I cannot test that.

comment:13 Changed 5 years ago by timmywil

Priority: undecidedlow
Status: newpending

Let's break it down. What is the final proposal here? Keep in mind that we're probably not going to gut the Deferred implementation.

comment:14 Changed 5 years ago by jaubourg

I still don't see any use-case that is not covered by the existing implementation as is. I explained why earlier. We should close this.

comment:15 Changed 5 years ago by dmethvin

Resolution: notabug
Status: pendingclosed

I think we should leave our existing implementation alone and not change its behavior. We already have a Promise discussion on the list for San Diego.

Note: See TracTickets for help on using tickets.