Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Use Q transparently over $q if available #1998

Closed
ghost opened this issue Feb 12, 2013 · 29 comments
Closed

Use Q transparently over $q if available #1998

ghost opened this issue Feb 12, 2013 · 29 comments

Comments

@ghost
Copy link

ghost commented Feb 12, 2013

Q is significantly more featured than $q and is often more useful for that reason. However, working with both Q and $q in the same project (e.g. using Q with $http calls, promise templates), can get very messy. It would be useful for $q to use the full Q library when available, like the behavior of jqlite and jquery.

@ccapndave
Copy link

Agreed - there are cases where $q just doesn't cut it and it would be great to get full Q integrated into angular.

@icholy
Copy link
Contributor

icholy commented Jun 5, 2013

Issue is that $q only resolves in the $digest cycle. So Q is not a drop in replacement.

@arhea
Copy link

arhea commented Jul 25, 2013

+1. I really need the notify function of q.

@cqr
Copy link
Contributor

cqr commented Oct 17, 2013

@isaacbw @ccapndave @arhea Right now the place where one would hook into Q to make sure resolution would trigger a $digest is private and inaccessible to Angular. What are the specific pieces of functionality that you are missing from Q that are motivating this request?

@arhea
Copy link

arhea commented Oct 17, 2013

@chrisrhoden I was looking for the progress api, which is now available. So that one has been checked off the list. Might be behind on this but does angular provide the shortcut methods like .fail .progress? Seems silly but, increases code readability rather than just .then with three anonymous functions.

@cqr
Copy link
Contributor

cqr commented Oct 17, 2013

@arhea you can use .catch instead of .fail. .progress hasn't been implemented yet, but I can write a pull request if that's what you're really looking for.

It's important to bear in mind that it's actually different calling promise.then(callback).catch(error) than promise.then(callback, error), in that the first creates 2 intermediate promises and the second only one. While I don't think that it's likely this will cause problems for you, for instance, the following will resolve to 3:

function addOne(n) {
  return n + 1;
}

function addTwo(n) {
  return n+2;
}

deferred.promise.catch(addTwo).then(addOne);
deferred.reject(0); // above promise resolves to 3

whereas this will resolve to 2:

deferred.promise.then(addOne, addTwo);
deferred.reject(0); // above promise resolves to 2

cqr added a commit to cqr/angular.js that referenced this issue Oct 17, 2013
Shorthand method for $q promises that allows attaching a progress
callback. Provides the same type of convenience as the existing
catch shorthand method, and matches functionality existing in the Q
library.

This should resolve the only remaining concern addressed in angular#1998

Closes angular#1998
@arhea
Copy link

arhea commented Oct 18, 2013

I am very familiar with promises. However, for supporting IE8 those method names do not work. They are not supported in ES3. That is why I prefer .fail(method) over .catch(method).

I know I can use promise['catch'](method) but, that is worse that just three methods.

Ideally, if angular is going to call it $q then it needs to be api compliant with the real Q library.

@cqr
Copy link
Contributor

cqr commented Oct 19, 2013

...so you think the name should be changed from $q?

On Fri, Oct 18, 2013 at 5:45 PM, Alex Rhea notifications@github.com wrote:

I am very familiar with promises. However, for supporting IE8 those method
names do not work. They are not supported in ES3. That is why I prefer
.fail(method) over .catch(method).

I know I can use promise'catch' but, that is worse that just
three methods.

Ideally, if angular is going to call it $q then it needs to be api
compliant with the real Q library.


Reply to this email directly or view it on GitHubhttps://github.com//issues/1998#issuecomment-26632170
.

chrisrhoden

@ghost
Copy link
Author

ghost commented Oct 19, 2013

I agree that it's more than a little misleading to name the library $q if it doesn't emulate at least a good portion of Q's functionality. Q is both a core promise library and a versatile utility kit. Perhaps some effort should be made to expand upon $q's offering, or, in lieu of that, consider renaming $q to something more generic like $promise, or $then?

Are there any objections to adding utility to $q? Are pull requests welcome?

@cqr
Copy link
Contributor

cqr commented Oct 19, 2013

I'm not any more affiliated with the project than you are, I just make an
effort to help clarify what a ticket is requesting and submit pull requests
when possible.

I can tell you that the motivation for $q's relatively limited
functionality is pretty plainly laid out in the documentation - bytes. If
you can add the rest of the functionality that you're requesting with no
additional byte overhead, I am sure that the Angular team would be
receptive. I can further tell you that there's an existing ticket that has
been approved for work regarding a refactoring of $q. I wish that it was
possible to do some monkey patching of Q such that it would serve as a
drop-in replacement for $q, but that's not possible. Maybe you could write
an alternative module that people could opt in to loading on the page which
would decorate $q.

The point I am driving at here is that the request originally made in this
ticket is simply not possible at this time, and its possibility is bound up
in Q giving us an extension point in their API. By asking for more
clarification beyond that, I am trying to see if there's anything
preventing this ticket from being closed. In case you hadn't noticed,
there's a very large amount of tickets, and that has a negative effect on
the ability of the Angular team to address each one.

If what you're saying is that, barring the ability to drop-in the Q library
(which we can't do, because of the way that the Q library is structured),
you want the $q service renamed or a major functional overhaul, you really
ought to file a new ticket. If you know what you want and how to make it,
you can skip straight to a pull request.

That having been said, I recommend this ticket be closed.

On Sat, Oct 19, 2013 at 12:10 PM, Isaac Wagner notifications@github.comwrote:

I agree that it's more than a little misleading to name the library $q if
it doesn't emulate at least a good portion of Q's functionality. Q is both
a core promise library and a versatile utility kit. Perhaps some effort
should be made to expand upon $q's offering, or, in lieu of that, consider
renaming $q to something more generic like $promise, or $then?

Are there any objections to adding utility to $q? Are pull requests
welcome?


Reply to this email directly or view it on GitHubhttps://github.com//issues/1998#issuecomment-26652854
.

chrisrhoden

@petebacondarwin
Copy link
Contributor

@arhea - It appears that where $q is different to Q is only in the syntactic sugar methods. In terms of real functionality, the AngularJS $q library is now Promises A+ compliant: See #3699 and #3693.

Until we are in a position to allow the developer to completely swap out the $q promises library with Q, I think there is little to do. Closing this ticket.

If you want to push for adding these syntactic sugar methods then you can offer a pull request, such as @chrisrhoden's for adding promise.progress().

@joshperry
Copy link

So if Q had a hook to push the $digest button then this is something that the team might be interested in accepting a PR for? It seems like the Angular team is constraining the feature-set of $q for the exact same reason that jqLite exists; bytes. So why would it not then follow that it would make sense to have $q be to Q as jqLite is to jQuery?

I would like to work on this but I would hate to spend a couple days of time for a PR to be turned down.

@cqr
Copy link
Contributor

cqr commented Feb 25, 2014

What features are missing, specifically?

On Tue, Feb 25, 2014 at 4:42 PM, joshperry notifications@github.com wrote:

So if Q had a hook to push the $digest button then this is something that
the team might be interested in accepting a PR for? It seems like the
Angular team is constraining the feature-set of $q for the exact same
reason that jqLite exists; bytes. So why would it not then follow that it
would make sense to have $q be to Q as jqLite is to jQuery?

I would like to work on this but I would hate to spend a couple days of
time for a PR to be turned down.

Reply to this email directly or view it on GitHubhttps://github.com//issues/1998#issuecomment-36061676
.

chrisrhoden

@joshperry
Copy link

Well, #6427 and #6087 to start with...

@cqr
Copy link
Contributor

cqr commented Feb 25, 2014

I don't think that these would be addressed by using Q transparently in
place of $q, would they?

@joshperry
Copy link

Absolutely, these two features are implemented by Q right now out-of-the-box. If $q was Q when Q was imported before angular than $q would also have those features.

@cqr
Copy link
Contributor

cqr commented Feb 25, 2014

OK, well then you are mistaken. If the API were the same, $q would not be turned into a constructor. You would still need to use $q.when(). Totally not a big deal, though, not sure why this is an issue.

As for #6087, hooking Q into the digest cycle in the same manner as $q would result in the same issue, but with Q instead of $q.

@joshperry
Copy link

$q could absolutely be a constructor function... $q is currently a simple hash with a few functions hanging off of it:

  return {
    defer: defer,
    reject: reject,
    when: when,
    all: all
  };

It could easily return something like:

  var qFunc = function(value) {
    return when(value);
  };

  qFunc.prototype = {
    defer: defer,
    reject: reject,
    when: when,
    all: all
  };
  return qFunc;

@cqr
Copy link
Contributor

cqr commented Feb 25, 2014

Yes, it could be, but it is not. I don't see how Q as a drop-in has any
impact here. If the $q API doesn't support this syntax, you can't use it.
If you feel strongly that the .when syntax is a hardship, then making
your feelings known on the referenced ticket is the way forward. I don't
see any connection to being able to back $q with Q.

I asked about missing features. It does not seem to me that Q has any
features that you feel $q is missing - just some sugar which is being
considered for addition to $q and some behavior that you feel is wrong.
We can't have the API for $q be the same while the behavior changes, so
this needs to be addressed by $q. Again, I don't see how dropping in Q
would help.

If you want Q promises, you can add Q to the page.

@joshperry
Copy link

Q implements #6087

http://jsfiddle.net/joshperry/e863S/

@joshperry
Copy link

I guess you will have to define the difference between a "feature" and "sugar" for me. When an API doesn't implement something, I call that a feature...

jqLite doesn't implement everything that jQuery does, that's the whole point. It is a subset. $q could easily (and almost is) a direct subset of Q. Any $q code you took and used in the scope of Q would just work. This is the exact same use-case as jqLite right now.

@joshperry
Copy link

If you aren't familiar with jqLite, the documentation defines what subset of jQuery it implements, which parameters are interpreted differently, what event parameters are different, and what and other limits jqLite has compared to when jQuery is included before angular.

There could easily be a similar documentation page for qLite relating to Q and have qLite be a strict subset of Q (which it almost already is).

@cqr
Copy link
Contributor

cqr commented Feb 25, 2014

Q does not suffer from the digest cycle, therefore the behavior is different when you're talking about the functionality described in #6087. That does not mean that Q has a feature that $q does not - in fact, it is because $q has the feature of bundling resolutions to avoid DOM flapping that the issue in #6087 presents itself. I know that this can be difficult to understand, but please try to understand rather than dismissing me on this point. I'm very familiar with the issue at hand.

I am very familiar with jqLite, and I am super glad that you brought it up so that I can explain how fundamentally different this situation is from that one. First, jqLite's API is a subset, not a superset of jQuery's.

Second, the syntax for jqLite-backed and jQuery-backed angular.element is the same.

angular.element('<div></div>'); // jqLite
angular.element('<div></div>'); // jQuery

Note how the above is not:

angular.element('<div></div>'); //jqLite
angular('<div></div>'); //jQuery

If the latter syntax were preferred, it would be necessary to implement it as part of jqLite as well as jQuery, so that the APIs that do the same thing would be accessed regardless of whether jQuery or jqLite was being used. Does this clarify why it is necessary to implement $q() as a shorthand for $q.when() before being able to use it? Does this make it clear why dropping in Q would not solve this issue?

Now, the second important thing is that, for the subset that jqLite implements, its behavior is the same as jQuery's. This means that, in:

angular.element('<div></div>').addClass('klass'); //jqLite
angular.element('<div></div>').addClass('klass'); //jQuery

... both lines will exhibit the same behavior! If the jqLite version replaced common misspellings in the addClass method, but the jQuery version did not, that would be a bug in jqLite, not a reason to add jQuery to the page, because the contract that Angular offers is that if your code works in jqLite, it will work with jQuery, and if it works with jQuery, it will work with jqLite or throw an error.

So again, what are the features that you feel that $q is missing? If you think that the API should be changed, great! But replacing $q with Q necessarily cannot solve that problem because whatever shim is used needs to use a superset of the $q syntax without alternative ways to use existing functionality (if you need an explanation for this stipulation, look above). If you think that the behavior of some feature that they share (i.e. promise resolution, and when it happens) is wrong in $q but right in Q, great! But it will need to be changed in $q so that we don't end up with weird code that works with $q but not Q. It looks like someone has opened a ticket that describes some stuff that they're confused about, and you should chime in there. I'm sure someone will be happy to explain the situation to you.

Here's the real punchline, though. We're not talking about adding more methods (like jQuery does), because $q isn't missing features - it just doesn't conform to the API that you expect. Unfortunately, it does conform to the API that lots of existing Angular.js code expects at this point, so that's not a problem we're going to solve by optionally dropping in Q - what if you couldn't use libraries anymore!

@joshperry
Copy link

Pardon my syntactical mistake with superset/subset, I've rectified them.

I guess I'll just have to create a PR so that I can explain myself in my native language. I understand your arguments but still fail to understand the limitations; perhaps 3/4 of the way through my PR I'll run into the hard issues that you are attempting to convey to me and I'll fade into oblivion. I enjoyed the discussion though, Thank you!

@joshperry
Copy link

Well, I spent a little time on this and got it working, here is what the change looks like for angular's q.js

diff --git a/src/ng/q.js b/src/ng/q.js
index e7ff26e..93ad6a1 100644
--- a/src/ng/q.js
+++ b/src/ng/q.js
@@ -171,6 +171,16 @@
 function $QProvider() {

   this.$get = ['$rootScope', '$exceptionHandler', function($rootScope, $exceptionHandler) {
+    // Use the Q library if available
+    if(typeof Q === 'function' && Q.nextTick && Q.nextTick.setRequestTick) {
+      Q.nextTick.setRequestTick(function(callback) {
+        $rootScope.$evalAsync(callback);
+      });
+
+      return Q;
+    }
+
+    // Use "qLite"
     return qFactory(function(callback) {
       $rootScope.$evalAsync(callback);
     }, $exceptionHandler);

This requires a small patch to Q as well, where if the community is interested I suppose we could submit to that project to gauge their interest.

diff --git a/q.js b/q.js
index f94737d..7f901b1 100644
--- a/q.js
+++ b/q.js
@@ -155,6 +155,10 @@ var nextTick =(function () {
         }
     };

+    nextTick.setRequestTick = function(newRequestTick) {
+        requestTick = function() { newRequestTick(flush); };
+    };
+
     if (typeof process !== "undefined" && process.nextTick) {
         // Node.js before 0.9. Note that some fake-Node environments, like the
         // Mocha test runner, introduce a `process` global without a `nextTick`.

If people are still interested in this, it is something I would be happy to put together an official PR for.

@caitp
Copy link
Contributor

caitp commented Feb 27, 2014

If we do this, then what are we going to do when someone wants to use bluebird or when or then/promise etc etc? I mean, I'm not sure I have a strong opinion on this, but I'm not totally sure it's worth it and possibly sets a precedent/expectation that all components are completely modular. Monkey-patching jQuery is bad enough, I don't really like the idea of monkey-patching another library too.

Keep in mind this is just my personal opinion which I haven't put significant time into contemplating, so maybe it is something we want to do. But it doesn't seem quite right IMO.

@joshperry
Copy link

I agree with you, I'm not totally sure this is worth the trouble. I think the biggest argument for this is that angular purposefully cloned Q's api to be byte conscious, exactly the same reason jqLite exists, they used the same name even. I see being able to drop in Q the same as being able to drop in jQuery; it gives you full API functionality if you want to spend the bytes.

I believe there would be a large amount of work to make this completely pluggable with another promise library and I agree that I don't see that as a valuable feature. $q was specifically written to be API compliant with Q (an API that, except for a small subset, is not based on a standard) and there is a lot of internal code that uses that API.

@gx0r
Copy link

gx0r commented Mar 12, 2014

+1

I don't think it's fair to say that pluggable with another promise library is not a valuable feature. For instance, Bluebird.js has excellent long stack traces a rich API that allows you to cancel promises, and it's performance is near that of regular callbacks.

@joshperry
Copy link

I don't think the discussion of value really applies here. The developers of Angular chose to be opinionated in the DOM library that it uses internally (jQuery), and the promise library it uses (Q). There is no abstraction layer to use Mootools or something similar in the place of jQuery, I don't think that it applies any more to the promise library implementation either.

The biggest problem with abstracting the promise or DOM libraries is that it would be limited to the least common denominator of all the libraries that wanted to be used in its place.

There is no reason you could not use another promise library alongside the included subset of Q that Angular uses internally; I would assume that Bluebird.js supports converting thenables just as Q does.

The point of this issue is to address the idea of allowing the full Q library to be used in the place of $q just like jQuery can be used in the place of jqLite when it is available before Angular is loaded.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants