Use of native promise for AngularJS implementation

[lyschoening]

@rolandjitsu I just saw that $q has been replaced with Promise in the AngularJS implementation for 1.0. Unless you know something I don't, this would completely break potion-node for AngularJS. It's not reasonable to have to manually call $scope.$apply() every time a promise is resolved.

[rolandjitsu]

@lyschoening I totally dismissed that, sorry, it's been a while since I worked with AngularJS. Though tests seems to pass, I checked if promises are resolved and in some cases it looks like they are not.

I will push a fix for this later on today.

Though, it's quite annoying that due to this issue with digest phases, we have to use a different promise impl. just for AngularJS (in the other cases, the native Promise impl. works without issues).

[rolandjitsu]

@lyschoening after I reviewed the changes I made I concluded that because of async/await the AngularJS implementation would not work. So the only way to fix this would be to revert back .fromPotionJSON() and other methods that use async/await.

I prefer the async/await as it's much cleaner though.

I'm not quite sure what to do in this case as AngularJS will at some point be deprecated (the team says at the point when more traffic is detected on the Angular side than AngularJS). We are already compatible and focused on the latest version of Angular (Angular 4) and I think we should continue that.

Furthermore, the codebase would be much cleaner without an impl. for AngularJS as we can use async/await and native promises.

I was thinking to make a new major version 2 or keep the current one (1 - because I did make a major release) which has removed support for AngularJS, but to be reasonable to the others using AngularJS keep the version that works with it as well (and patch it when we add new features or bug fixes). In NPM we can have a next and latest tag, where next would be the version which has no support for legacy AngularJS and the latest has support for legacy AngularJS.

Or we can just not use async/await at all and revert back (I'm actually working on reverting back some of the methods right now).

Let me know what you think (meanwhile I'm working on fixing the AngularJS version, meaning I'm removing async/await).

[lyschoening]

@rolandjitsu the easiest solution, unfortunately, is probably simply not to use async/await.

It would be possible to work around by using async/await internally and only wrapping the result in a $q promise. However, I am not sure if it will make things all that much more legible. In particular .fromPotionJSON() as it is written in async/await right now might be more legible, but it is no longer parallel. Before, all requests got added to a promise array and resolved in the end, meaning that multiple requests could be made simultaneously (one of the key features of potion, which relies heavily on parallel requests). This can be fixed by still having the promises array and doing await Promise.all(promises), of course, but my point here is that async/await as syntactic sugar needs to be treated with caution in a library like this one. Perhaps some explicitness isn't so bad.

We are still going to have some AngularJS code for much of this year. We can of course fix our dependency to 0.25.x as it works fine for us, but it also seems premature to break compatibility now over such a small thing.

[rolandjitsu]

@lyschoening This seems reasonable to me and makes more sense to make/keep . fromPotionJSON() non-blocking.

It was kind of premature, as I actually forgot altogether about AngularJS's change detection system. I was facing an issue with Angular's AOT for static properties (the promise on Potion being on of them) when I tried to build for Angular and at the time I implemented the fix I did not consider AngularJS.

I'm working right now on a fix for AOT for Angular and revert back to not using async/await so that it's still compatible with AngularJS.

Thanks for spotting this issue as the tests never caught it (I'll have to add some cases for this as well).

[rolandjitsu]

@lyschoening I pushed a bunch of changes and published a patch, could you give it a try and let me know (when you have time) if there are any issues when using with AngularJS?

[rolandjitsu]

I'll assume there are no issues because of using the native Promise anymore since I switched back to using the AngularJS provided one in case the lib is used with AngularJS. Let me know if the issue still persists, but I'll close for now.