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

Drop non-compliant Promises/A+ progress/notify on $q #12533

Closed
pocesar opened this issue Aug 9, 2015 · 11 comments
Closed

Drop non-compliant Promises/A+ progress/notify on $q #12533

pocesar opened this issue Aug 9, 2015 · 11 comments

Comments

@pocesar
Copy link
Contributor

pocesar commented Aug 9, 2015

Makes it really hard to replace $q when using a compliant library, since angular uses progress/notify handlers on core itself.

@lgalfaso
Copy link
Contributor

Would it be possible to know what library are you trying to use? The extra
parameters should be ignored, so more info is needed

@pocesar
Copy link
Contributor Author

pocesar commented Aug 10, 2015

I managed to replace $q with bluebird, and set the scheduler to $rootScope.$applyAsync. I was trying to use $interval but uses notify. Another module included in my project also uses notify from $http.
I implemented the defer method manually, along with when.

        Bluebird.defer = function() {
            var dfd: any = {};

            dfd.promise = new Bluebird(function(_resolve, _reject) {
                dfd.resolve = _resolve;
                dfd.reject = _reject;
                dfd.notify = angular.noop;
            });

            return dfd;
        };

        var bb: any = Bluebird;
        bb.when = Bluebird.resolve.bind(Bluebird);

        var originalAll = Bluebird.all;

        Bluebird.all = function(promises: any|any[]) {

            if (angular.isObject(promises) && !angular.isArray(promises)) {
                return Bluebird.props(promises);
            } else {
                return originalAll.call(Bluebird, promises);
            }

        };

        Bluebird.onPossiblyUnhandledRejection(angular.noop);

        function BluebirdConstructor(cb: any): any {
            if (!(this instanceof Bluebird)) {
                return new Bluebird(cb);
            }
            return Bluebird;
        }

        angular.extend(BluebirdConstructor, Bluebird);

        $provide.decorator('$q', function() {
            return BluebirdConstructor;
        });

        $q.onPossiblyUnhandledRejection(function(err: any) {
            if (__DEBUGGING) {
                $log.error(err);
            }
        });

        Bluebird.setScheduler(function(cb) {
            $rootScope.$applyAsync(cb);
        });

@Rush
Copy link

Rush commented Aug 15, 2015

I was hit by this problem as well. @pocesar have you been able to fix $interval. It seems to me I would need to replace the whole service to make it work.

@lgalfaso
Copy link
Contributor

would it be possible to have a plunker that shows the entire problem?

@Agamnentzar
Copy link

http://plnkr.co/edit/ITbUfnXOogSdO2r1w52J?p=preview

If you remove 'promise' module from dependencies you can see interval ticking as expected

The problem seems to be $interval depending on promise progress notification that is no longer part of promises

@lgalfaso
Copy link
Contributor

lgalfaso commented Nov 4, 2015

@Agamnentzar thanks for the plunker. I think there should be an easy fix.

@Rush
Copy link

Rush commented Nov 4, 2015

btw. a dumb/simplified workaround for $interval needing notify is simply swapping it with a custom one:

  app.config(function($provide) {
    $provide.decorator('$interval', function($delegate, $rootScope) {
      var $newInterval = function(fn, timeout) {
        if(arguments.length !== 2 || typeof fn !== 'function' || typeof timeout !== 'number') {
          return console.error(new Error('custom $interval supports only 2 arguments, Function and timeout'));
        }

        var interval = setInterval(function() {
          $rootScope.$apply(fn);
        }, timeout);

        return interval;
      };

      $newInterval.cancel = function(handle) {
        clearInterval(handle);
      };

      return $newInterval;
    });
  });

@wesleycho
Copy link
Contributor

wesleycho commented Jun 1, 2016

Given that dropping the API is a breaking change to $q, it probably is safe to close this issue unless it is desired to remove it completely from the codebase. To fully remove notify usage from $interval would require a breaking change as well (under the assumption someone is hooking into the notifications from the interval promise).

Not sure how it is felt, but IMO it probably isn't worth removing since this is a more uncommon case with simple workarounds.

@gkalpak
Copy link
Member

gkalpak commented Jun 3, 2016

@lgalfaso, WDYT (since this is assigned to you) ?

@lgalfaso
Copy link
Contributor

lgalfaso commented Jun 3, 2016

Given that the issue with $interval is fixed, I would close this issue.

On Friday, June 3, 2016, Georgios Kalpakas notifications@github.com wrote:

@lgalfaso https://github.com/lgalfaso, WDYT (since this is assigned to
you) ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12533 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAX44urCgJAgQv8yD7cOULKn6IAoNEBfks5qIDWogaJpZM4FoUuu
.

@gkalpak
Copy link
Member

gkalpak commented Jun 3, 2016

Thx @lgalfaso!

So, closing this (since we are not going to remove the notify method), but since Angular itself does not rely on notify for any internal fnctionality any more, you should be able to substitute $q for other promise libraries (e.g. as described in #12533 (comment)).

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