Be explicit about angular vs native promises

[stacey-gammon]

Promises: Native vs Angular

When to use Which

A common practice in our code base is to pass around and use a custom angular Promise class.

This used to make sense because most of our code was written in angular, and angular promises inject digest cycles to keep the UI updating smoothly.

As we move into a mixed framework environment and strive to separate out the business logic so it can be used by multiple systems, requiring a Promise class to be passed around is onerous and creates a strange API, especially from a react perspective. It can also start leading to subtle bugs (see "Why convert now" below).

Instead of continuing down this path, I propose we stop passing Promise classes into framework agnostic code and start making sure native promises used in angular are wrapped in the Angular Promise library in order to trigger the digest cycles.

Why convert now

When native promises are used in angular, it can lead to subtle bugs that are difficult to track down, so why stop our current defensive approach? Due to our growing mixed framework environment, using angular promises everywhere will start to pose problems.

Consider the hypothetical situation where we have two different UI components that both add tasks to a queue - one is written in angular and one in react. The queue itself is framework agnostic. Then we have a third UI component in angular that displays the results of these tasks. The tasks contain promises so the angular component uses .then to update results on scope.

Because you could push tasks onto the queue in both react and angular, you’d likely end up with a mixture of promise types - both angular and native Promises. If we allow plugins to push to this queue, some might even be Bluebird.

If we don’t start requiring our angular specific code to handle non-angular promises, this would introduce subtle UI bugs. Some tasks would update the UI, some tasks upon resolution wouldn’t.

As an added bonus we could start taking advantage of async/await in more places.

Examples

When to use native promises

We should use native promises in classes that are not specifically tied to angular and may be used in non-angular code. For example:

src/core_plugins/tagcloud/public/tag_cloud.js
While the code is currently only used in angular, it’s written in a framework agnostic way, nothing specifically angular about it. It uses async and await via native promises.

Where it’s referenced in the angular controller, we have to be sure to wrap any promises in an angular promise so the digest loops are triggered.

src/ui/public/vis/response_handlers/basic.js
The response handlers are meant to be used by any framework, so they should continue to use native promises. When they are used in angular code, they should be wrapped in an angular promise.

When to use angular promises

src/ui/public/vis/vis_types/angular_vis_type.js

This file is specifically angular, hence the name. It sets a bunch of variables on scope and expects the data to update automatically. Instead of using native promises and a manual $scope.apply, it should wrap the esResponse in an angular promise

Spotting a Promise Issue

Issues with using native promises in angular usually appear as some part of the UI not automatically updating. For example, I noticed one particular error when I changed the interval dropdown in discover, but the chart didn’t update until I moved the mouse over it, causing a re-render.

Summary

We will be taking on some risk with this, as issues relating to promises, and missed digest loops, can be subtle, hard to spot, and hard to track down. But given the direction we want to take our code base, I think it’s a risk we should take. Rather than be defensive and use angular promises everywhere, we should learn when they are necessary, and be explicit about it.

[rhoboat]

Instead of continuing down this path, I propose we stop passing Promise classes into framework agnostic code and start making sure native promises used in angular are wrapped in the Angular Promise library in order to trigger the digest cycles.

Is this the only way to ensure that angular UI code always contains only Angular Promises?

Consider the hypothetical situation where we have two different UI components that both add tasks to a queue - one is written in angular and one in react. The queue itself is framework agnostic. Then we have a third UI component in angular that displays the results of these tasks. The tasks contain promises so the angular component uses .then to update results on scope.

So the consumer is ultimately an Angular component, which is why the native promises need to be wrapped in Angular Promise (to kick off digest and update the UI). I think you're saying that any UI code that consumes promises needs to be able to detect the type of promise that's being passed and Angularize it if needed.

I agree that a solution to this problem is needed, and it's an interesting problem, too. At the outset, this seems like a reasonable and sensible way to do it. I'm also interested in hearing other solutions/approaches. (And hopefully mulling it over and contributing some.)

[stacey-gammon]

Is this the only way to ensure that angular UI code always contains only Angular Promises?

@archanid - I actually started exploring another option of automatically inserting digest loops into bluebird and then we could use bluebird everywhere: #13674. Though we don't really want to use bluebird, we want to use native, but I was just seeing if I could do it with bluebird first, then I'd see if I could do the same thing for native (though it didn't look easy, if at all possible).

Then I decided that seemed a little dangerous, or maybe just too subtle. What if you go into angular, then go into react, I guess the digest loops are still called when using bluebird? I wasn't sure. I ended up abandoning that approach and figured it was best to be explicit about it.

I think you're saying that any UI code that consumes promises needs to be able to detect the type of promise that's being passed and Angularize it if needed.

Well, that, or we just always angularize it and assume it's not an Angular promise, if it's not coming directly from angular code. Otherwise it might get tricky trying to detect this. Tricky and add some overhead.

[azasypkin]

I agree with the proposal too. It may require some discipline at the beginning, but benefits are obvious. It will become easier once we have Typescript and method signatures with clear indication of the promise type they expect (well at least for the framework-agnostic code).

As a side note I'd personally not add Bluebird into this "promise mix" and let it die, as soon as possible. But it's just me, not sure what is the team agreement here :)

[stacey-gammon]

As a side note I'd personally not add Bluebird into this "promise mix" and let it die, as soon as possible. But it's just me, not sure what is the team agreement here :)

@azasypkin - team agreement is also to get rid of bluebird - #9078. Just hasn't been added to the style guide yet. I was only experimenting to see what was possible. :)

[epixa]

+100

This pattern of being especially considerate of the types of data we're exposing on our APIs for other plugins to consume is critical for our long term plans for the project. We're already exploring how to do almost exactly what you're describing here only for observables in the new platform.

[sorenlouv]

I like where this is heading.
It's also worth noting that the $digest trigger in Angular promises is a side-effect. Side-effect free code is often easier to reason about, and less error-prone so I salute the move to native promises with explicit digest calls :)

[w33ble]

So the desire here is to not pass in Promise to services, which sounds like a good idea. I'm inclined to say I'm on board here, but I just want to make sure I understand what you're proposing.

It sounds like you're advocating for something like this in an Angular controllers/directives:

// get the Promise service, or maybe even just $q
const $Promise = $injector.get('Promise'); 

// use some agnostic service
const res = someNativePromiseService().then(/* whatever other logic */);

// and wrap that native promise in an angular promise
$Promise.resolve(res);

Is that right?

[stacey-gammon]

Yep @w33ble, that is the proposal. Sounds like @sqren is suggesting an alternative approach, and being explicit about the digest calls, instead of "wrapping" them in angular promises.

@spalger prefers wrapping to explicit digest calls, and I defer to him since I don't have a strong opinion either way in that matter. My main objective of this proposal is to get make sure our framework agnostic code uses native promises, not custom Promise classes passed in.

[spalger]

// get the Promise service, or maybe even just $q
const $Promise = $injector.get('Promise'); 

I really like this notation

[stacey-gammon]

Another suggestion, if you can't use, or it's more awkward to use Promises, is to prefer rootScope.evalAsync over rootScope.apply.

e.g. instead of:

async function myAsyncFunctionInAngular() {
  await doNativeAsyncThing();
  $rootScope.$apply();
}

await myAsyncFunctionInAngular();

do:

async function myAsyncFunctionInAngular() {
   $rootScope.$evalAsync(doNativeAsyncThing);
}

await myAsyncFunctionInAngular();

The reasoning, copied from another issue:

After discussing with @spalger his main concern with calling $rootScope.$apply() explicitly is that it shouldn't be called if we're within a digest-loop already so we should technically be calling if (!$rootScope.$$phase) $rootScope.$apply() which is horrible. That's why he prefers using Promises normally because it doesn't make us have to worry about this, and Promises are tied to the scope that they're created in so it doesn't cause the entire root scope to dirty-check.

Our use of $http to get the jobs returns and AngularJS promise but since we're using await it's breaking us out of the digest-loop before processing the next-lines where we then call $rootScope.$apply which is why this is working presently, but it's not a good assumption to make. Instead, per @spalger's kind suggestion, we're using $rootScope.$evalAsync to schedule this to be run outside of the current digest loop, if there ever is one, in it's own digest loop since we're technically updating a scope that isn't ours to make the notifications show up.

[stacey-gammon]

Final decision is to start replacing angular promises with native promises in framework agnostic, shared code. Going to go ahead and close as the decision has been made.