Skip to content

Automatic unwrapping of promises by $parse severely limits its usefulness #4158

Closed
wilsonjackson opened this Issue Sep 25, 2013 · 45 comments
@wilsonjackson

The behavior introduced by 3a65822, which automatically unwraps promises returned by functions invoked by $parse, prevents any consumer of an expression from handling promise resolution on its own. More pertinent discussion of the problem exists here: #3503

Notably this breaks the typeahead directive in UI bootstrap, as per this issue: angular-ui/bootstrap#949

Here's a simple example of why this change is problematic.

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

The directive in that plunk executes a function on click (in the same way as ng-click) and expects a promise to be returned, which it will use to decorate the link with loading text.

Because $parse unwraps the promise, the directive has no chance to add its own handlers. If you switch the Angular version to 1.2.0rc1, it works as expected. I don't believe this is an uncommon use case, and certainly not one the framework should prevent from being possible. I hope this can be resolved by the next RC.

@wilsonjackson wilsonjackson referenced this issue Sep 25, 2013
@jussik jussik fix($parse): handle promises returned from parsed function calls
When a parsed function call returns a promise, the evaluated value
is the resolved value of the promise rather than the promise object.

Closes #3503
3a65822
@IgorMinar
Angular member

would it be sufficient if $parse took an optional argument that would tell it to not unwrap promises?

@IgorMinar
Angular member

@jankuca your PR #3681 could handle this in an easier way than the current closure based code. could you take a look at this and create a commit on top of that PR that would implement the promise unwrapping flag?

@jankuca
jankuca commented Sep 27, 2013

Sure, I'm going to look into it.

@jankuca jankuca was assigned Sep 27, 2013
@wilsonjackson

Sure, an optional argument would solve the problem, though to me opting in to promise unwrapping would make more sense than opting out.

@IgorMinar
Angular member

@wilsonjackson fair enough. we'll consider it

@jussik
jussik commented Sep 29, 2013

I mentioned this in my PR that this issue stemmed from, but please make sure that functions returning promises are processed the same way by $parse as promise objects in scope. I don't mind unwrapping being opt-in, but keep in mind that promise objects have been unwrapped automatically for a long time now.

@pkozlowski-opensource
Angular member

@IgorMinar an optional opt-in argument would be a perfect solution. We really need it badly for the angular-ui/bootstrap. @jankuca if I help anyhow I could give you a hand with this issue, I'm really keen on doing anything that would speed up this issue resolution.

@wilsonjackson

I agree that promise objects should be unwrapped (or not) according to the same rules whether they result from a function call or not.

I'd argue that promise unwrapping is an "extra" feature that is not intrinsically related to parsing, so it would make the most sense for it not to happen by default. Also, the time to introduce breaking changes is now. I think this is the perfect time to correct this slight overreach in $parse's implementation.

@jankuca
jankuca commented Sep 30, 2013

I did some work regarding this issue: jankuca@dddc774...promise_unwrapping

But we need to get #3681 first.

@georgiosd

I'll second that this is a pretty big breaking change and it doesn't allow for knowing when the promise is resolved for actions.

And now I'm between a rock and a hard place, as RC1 has broken ngIncludes and RC2 has this broken :S

Does anyone know what direction you'll take after all so I can at least work around only one of the issues?

@georgiosd

Also note that this affects the behavior of directive & bindings and any options you add need to be able to be applied there too...

@ericpanorel

Agreed @georgiosd . Spent about half a day debugging this.

@petebacondarwin
Angular member

How about we move the promise unwrapping into the attributeInterpolationDirective, since that is really the only place where it should be expected to automatically unwrap? If you are working with promises in code then you should have full control.

@jussik
jussik commented Oct 3, 2013

@petebacondarwin I'm unfamiliar with the attributeInterpolationDirective, would that work transparently with ng-repeat="entry in promise"?

Another thing that might be worth looking into is having the unwrapping option stored in the promise itself. So if we want the promise to be unwrapped we have to flip an opt-in property before it resolves (or at least before the next $parse).

@xrg
xrg commented Oct 3, 2013

Another idea: define an $unwrap() function and let the devel. take full control. Lean, simple to understand.

@wilsonjackson

@jussik I don't like the idea of the promise itself having to be modified. The concern of whether a promise is unwrapped or not belongs entirely to the code initiating a parse. A scope function that returns a promise should make no assumption about how that promise will be used.

@georgiosd

I like @xrg 's idea - leave it to the programmer. One less bit of magic is probably good.

@wilsonjackson

Some of the suggestions here, when contrasted with the current behavior, bring up an interesting point. Whereas an $unwrap service or similar would only be able to unwrap a promise if it's the end result of an expression (e.g., returnPromise()), the current behavior will unwrap a promise resulting from a function call at any point within an expression (e.g., scopeVar = returnPromise()") or even more insidious consumePromise(returnPromise())).

That being the case, I don't see how anything but a flag passed to $parse would allow compatibility with current behavior to be maintained. Whether or not retaining compatibility for use cases such as those is a good idea I leave for others to debate. I only care about being given the option of not auto-unwrapping promises.

@xrg
xrg commented Oct 4, 2013

An $unwrap() /function/ would cover all of the above cases AFAICT, with really visible logic. The original idea had been that we want to unwrap whenever it's the snapshot value (rather than the blocking result) we are interested in, right?

So, we could write: currentVal = unwrap(somePromise()) or consumeValue(unwrap(somePromise())) having a full manual control of where a promise and where its value is used. Note that we would have to expose unwrap to the scope of parsing (or should it be always defined? ).

@petebacondarwin
Angular member
@xrg
xrg commented Oct 4, 2013

... or it should be ditched altogether. Personally I prefer the latter ...

++
Agreed.

@georgiosd

I guess @petebacondarwin is right. +1 on ditching :)

@tanshu
tanshu commented Oct 4, 2013

+1 on ditching

@jussik
jussik commented Oct 4, 2013

Yeah, ditching the whole thing certainly makes the process clearer, which I'm definite in favour of. I'm just concerned about all of the 1.0 and 1.1 code that would break if automatic unwrapping was removed. It would likely be a lot more than the amount that broke by adding support for unwrapping functions that return promises.

@georgiosd

@jussik The particular changes discussed in this thread were introduced after RC1.
Correct me if I'm wrong but I think everyone is talking about ditching that, not previous support.

@petebacondarwin
Angular member
@jussik
jussik commented Oct 4, 2013

I'm personally fine with either ditching all or keeping all (preference leaning towards ditching), but I don't see inconsistent behaviour as a valid solution (hence my PR that instigated this discussion).

As @wilsonjackson said:

I agree that promise objects should be unwrapped (or not) according to the same rules whether they result from a function call or not.

The commits that @jankuca has referenced also affect both loose promise objects and functions that return promises.

@georgiosd

Would ditching it and keeping existing behavior based on a "compatibility flag" be an acceptable solution for current apps that use this? To be deprecated completely in another major release?

@wilsonjackson

+1 for ditching.

If a user wants promises unwrapped, that can easily be got around by adding a $rootScope.unwrap function as per @xrg's suggestion. It seems like something Angular core (and especially the $parse service) shouldn't concern itself with.

@IgorMinar
Angular member

as @petebacondarwin unwrap() doesn't work for the case of: {{unwrap(some.path.with.promise.somewhere.in.the.middle)}} because unwrap would be called with undefined if there was promise somewhere not-at-the-end of the path.

@andreas-gruenbacher

Asynchronous validation broken with Angular 1.2.0-rc2:
angular-ui/ui-utils#120

@georgiosd
@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Oct 7, 2013
@IgorMinar IgorMinar fix($parse): make promise unwrapping opt-in
Previously promises found anywhere in the expression during expression
evaluation would evaluate to undefined while unresolved and to the
fulfillment value if fulfilled.

This is a feature that didn't prove to be wildly useful or popular,
primarily because of the dichotomy between data access in templates
(accessed as raw values) and controller code (accessed as promises).

In most code we ended up resolving promises manually in controllers
anyway and thus unifying the model access there.

Other downsides of automatic promise unwrapping:

- when building components it's often desirable to receive the
  raw promises
- adds complexity and slows down expression evaluation
- makes expression code pre-generation unattractive due to the
  amount of code that needs to be generated
- makes IDE auto-completion and tool support hard
- adds too much magic

Closes #4158
Closes #4270
978d4a3
@IgorMinar
Angular member

guys, check out #4317

@IgorMinar IgorMinar added a commit to IgorMinar/angular.js that referenced this issue Oct 9, 2013
@IgorMinar IgorMinar revert: fix($parse): handle promises returned from parsed function calls
This reverts commit 3a65822.

The change cased regressions in third party components that require
promises from getter functions not to be unwrapped.

Since we have deprecated the promise unwrapping support in $parse it
doesn't make much sense to fix this issue and deal with regressions in
third party code.

Closes #4158
4c112f5
@IgorMinar IgorMinar added a commit that closed this issue Oct 9, 2013
@IgorMinar IgorMinar fix($parse): deprecate promise unwrapping and make it an opt-in
This commit disables promise unwrapping and adds
$parseProvider.unwrapPromises() getter/setter api that allows developers
to turn the feature back on if needed. Promise unwrapping support will
be removed from Angular in the future and this setting only allows for
enabling it during transitional period.

If the unwrapping is enabled, Angular will log a warning about each
expression that unwraps a promise (to reduce the noise, each expression
is logged only onces). To disable this logging use
`$parseProvider.logPromiseWarnings(false)`.

Previously promises found anywhere in the expression during expression
evaluation would evaluate to undefined while unresolved and to the
fulfillment value if fulfilled.

This is a feature that didn't prove to be wildly useful or popular,
primarily because of the dichotomy between data access in templates
(accessed as raw values) and controller code (accessed as promises).

In most code we ended up resolving promises manually in controllers
or automatically via routing and unifying the model access in this way.

Other downsides of automatic promise unwrapping:

- when building components it's often desirable to receive the
  raw promises
- adds complexity and slows down expression evaluation
- makes expression code pre-generation unattractive due to the
  amount of code that needs to be generated
- makes IDE auto-completion and tool support hard
- adds too much magic

BREAKING CHANGE: $parse and templates in general will no longer
automatically unwrap promises. This feature has been deprecated and
if absolutely needed, it can be reenabled during transitional period
via `$parseProvider.unwrapPromises(true)` api.

Closes #4158
Closes #4270
117f4dd
@IgorMinar IgorMinar closed this in 117f4dd Oct 9, 2013
@pkozlowski-opensource
Angular member

Yey, awesome! But, @IgorMinar I can't see those commits in master:
https://github.com/angular/angular.js/commits/master

I guess I'm just blind or something is messed up with the git repo....

@petebacondarwin
Angular member

I think they broke something and got reverted...

@IgorMinar IgorMinar added a commit that referenced this issue Oct 9, 2013
@IgorMinar IgorMinar fix($parse): deprecate promise unwrapping and make it an opt-in
This commit disables promise unwrapping and adds
$parseProvider.unwrapPromises() getter/setter api that allows developers
to turn the feature back on if needed. Promise unwrapping support will
be removed from Angular in the future and this setting only allows for
enabling it during transitional period.

If the unwrapping is enabled, Angular will log a warning about each
expression that unwraps a promise (to reduce the noise, each expression
is logged only onces). To disable this logging use
`$parseProvider.logPromiseWarnings(false)`.

Previously promises found anywhere in the expression during expression
evaluation would evaluate to undefined while unresolved and to the
fulfillment value if fulfilled.

This is a feature that didn't prove to be wildly useful or popular,
primarily because of the dichotomy between data access in templates
(accessed as raw values) and controller code (accessed as promises).

In most code we ended up resolving promises manually in controllers
or automatically via routing and unifying the model access in this way.

Other downsides of automatic promise unwrapping:

- when building components it's often desirable to receive the
  raw promises
- adds complexity and slows down expression evaluation
- makes expression code pre-generation unattractive due to the
  amount of code that needs to be generated
- makes IDE auto-completion and tool support hard
- adds too much magic

BREAKING CHANGE: $parse and templates in general will no longer
automatically unwrap promises. This feature has been deprecated and
if absolutely needed, it can be reenabled during transitional period
via `$parseProvider.unwrapPromises(true)` api.

Closes #4158
Closes #4270
5dc35b5
@IgorMinar IgorMinar added a commit that referenced this issue Oct 9, 2013
@IgorMinar IgorMinar revert: fix($parse): handle promises returned from parsed function calls
This reverts commit 3a65822.

The change cased regressions in third party components that require
promises from getter functions not to be unwrapped.

Since we have deprecated the promise unwrapping support in $parse it
doesn't make much sense to fix this issue and deal with regressions in
third party code.

Closes #4158
b6a37d1
@IgorMinar
Angular member

they are in master now. there was an issue with IE8, but that's fixed now.

@andreas-gruenbacher

I still don't see a fix with that or a similar title on master. @IgorMinar, has this really been fixed in a completely different commit?

@AlmogBaku

This is a great feature!! why to take it down?

@thorn0
thorn0 commented Oct 23, 2013

Please consider introducing a custom operator (#, @ or something else) to make promise unwrapping explicit.
For example:
1) ng-options="item.name for item in items#"
2) {{items#[0].name}}

@jussik
jussik commented Oct 23, 2013

Both cases can be implemented using a function which unwraps the promise (i.e. unwrap(items) instead of items#). The function would be something along the lines of:

if (!('$$v' in promise)) {
    promise.$$v = undefined;
    promise.then(function(val) { promise.$$v = val; });
}
return promise.$$v;
@thorn0
thorn0 commented Oct 23, 2013

That's true, but I liked the conciseness of the deprecated functionality. Would be nice to have both conciseness and explicitness.

@georgiosd

Just inject the above function to your $rootScope using a name of your choice, et voila!

@thorn0
thorn0 commented Oct 23, 2013

I understand this, but promises are an important concept across the framework, and so having a special support for them in expressions seems logical and consistent. At least for me. Also this support would make it somewhat easier for people new to Angular to start using promises.

@petebacondarwin
Angular member
@benol
benol commented Oct 24, 2013

+1 to to a special operator or providing an unwrap function by default. This just saves typing in simple cases (and a lot of cases are simple). What exactly do I gain from

$scope.numbers = undefined;
service.getNumbers().then(function(numbers) {
$scope.numbers = numbers;
});

as opposed to

$scope.numbers = service.getNumbers()

??? I don't mind making the unpacking explicit in the templates.

@jamesdaily jamesdaily added a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
@IgorMinar IgorMinar fix($parse): deprecate promise unwrapping and make it an opt-in
This commit disables promise unwrapping and adds
$parseProvider.unwrapPromises() getter/setter api that allows developers
to turn the feature back on if needed. Promise unwrapping support will
be removed from Angular in the future and this setting only allows for
enabling it during transitional period.

If the unwrapping is enabled, Angular will log a warning about each
expression that unwraps a promise (to reduce the noise, each expression
is logged only onces). To disable this logging use
`$parseProvider.logPromiseWarnings(false)`.

Previously promises found anywhere in the expression during expression
evaluation would evaluate to undefined while unresolved and to the
fulfillment value if fulfilled.

This is a feature that didn't prove to be wildly useful or popular,
primarily because of the dichotomy between data access in templates
(accessed as raw values) and controller code (accessed as promises).

In most code we ended up resolving promises manually in controllers
or automatically via routing and unifying the model access in this way.

Other downsides of automatic promise unwrapping:

- when building components it's often desirable to receive the
  raw promises
- adds complexity and slows down expression evaluation
- makes expression code pre-generation unattractive due to the
  amount of code that needs to be generated
- makes IDE auto-completion and tool support hard
- adds too much magic

BREAKING CHANGE: $parse and templates in general will no longer
automatically unwrap promises. This feature has been deprecated and
if absolutely needed, it can be reenabled during transitional period
via `$parseProvider.unwrapPromises(true)` api.

Closes #4158
Closes #4270
34d9232
@jamesdaily jamesdaily added a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
@IgorMinar IgorMinar revert: fix($parse): handle promises returned from parsed function calls
This reverts commit 3a65822.

The change cased regressions in third party components that require
promises from getter functions not to be unwrapped.

Since we have deprecated the promise unwrapping support in $parse it
doesn't make much sense to fix this issue and deal with regressions in
third party code.

Closes #4158
7a6e821
@jamesdaily jamesdaily added a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
@IgorMinar IgorMinar fix($parse): deprecate promise unwrapping and make it an opt-in
This commit disables promise unwrapping and adds
$parseProvider.unwrapPromises() getter/setter api that allows developers
to turn the feature back on if needed. Promise unwrapping support will
be removed from Angular in the future and this setting only allows for
enabling it during transitional period.

If the unwrapping is enabled, Angular will log a warning about each
expression that unwraps a promise (to reduce the noise, each expression
is logged only onces). To disable this logging use
`$parseProvider.logPromiseWarnings(false)`.

Previously promises found anywhere in the expression during expression
evaluation would evaluate to undefined while unresolved and to the
fulfillment value if fulfilled.

This is a feature that didn't prove to be wildly useful or popular,
primarily because of the dichotomy between data access in templates
(accessed as raw values) and controller code (accessed as promises).

In most code we ended up resolving promises manually in controllers
or automatically via routing and unifying the model access in this way.

Other downsides of automatic promise unwrapping:

- when building components it's often desirable to receive the
  raw promises
- adds complexity and slows down expression evaluation
- makes expression code pre-generation unattractive due to the
  amount of code that needs to be generated
- makes IDE auto-completion and tool support hard
- adds too much magic

BREAKING CHANGE: $parse and templates in general will no longer
automatically unwrap promises. This feature has been deprecated and
if absolutely needed, it can be reenabled during transitional period
via `$parseProvider.unwrapPromises(true)` api.

Closes #4158
Closes #4270
827d94b
@jamesdaily jamesdaily added a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
@IgorMinar IgorMinar revert: fix($parse): handle promises returned from parsed function calls
This reverts commit 3a65822.

The change cased regressions in third party components that require
promises from getter functions not to be unwrapped.

Since we have deprecated the promise unwrapping support in $parse it
doesn't make much sense to fix this issue and deal with regressions in
third party code.

Closes #4158
2c15870
@jesselpalmer jesselpalmer pushed a commit that referenced this issue Sep 24, 2014
@IgorMinar IgorMinar fix($parse): deprecate promise unwrapping and make it an opt-in
This commit disables promise unwrapping and adds
$parseProvider.unwrapPromises() getter/setter api that allows developers
to turn the feature back on if needed. Promise unwrapping support will
be removed from Angular in the future and this setting only allows for
enabling it during transitional period.

If the unwrapping is enabled, Angular will log a warning about each
expression that unwraps a promise (to reduce the noise, each expression
is logged only onces). To disable this logging use
`$parseProvider.logPromiseWarnings(false)`.

Previously promises found anywhere in the expression during expression
evaluation would evaluate to undefined while unresolved and to the
fulfillment value if fulfilled.

This is a feature that didn't prove to be wildly useful or popular,
primarily because of the dichotomy between data access in templates
(accessed as raw values) and controller code (accessed as promises).

In most code we ended up resolving promises manually in controllers
or automatically via routing and unifying the model access in this way.

Other downsides of automatic promise unwrapping:

- when building components it's often desirable to receive the
  raw promises
- adds complexity and slows down expression evaluation
- makes expression code pre-generation unattractive due to the
  amount of code that needs to be generated
- makes IDE auto-completion and tool support hard
- adds too much magic

BREAKING CHANGE: $parse and templates in general will no longer
automatically unwrap promises. This feature has been deprecated and
if absolutely needed, it can be reenabled during transitional period
via `$parseProvider.unwrapPromises(true)` api.

Closes #4158
Closes #4270
f8baac3
@jesselpalmer jesselpalmer pushed a commit that referenced this issue Sep 24, 2014
@IgorMinar IgorMinar revert: fix($parse): handle promises returned from parsed function calls
This reverts commit 3a65822.

The change cased regressions in third party components that require
promises from getter functions not to be unwrapped.

Since we have deprecated the promise unwrapping support in $parse it
doesn't make much sense to fix this issue and deal with regressions in
third party code.

Closes #4158
2bfc714
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.