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

$watching functions which return a deferred/promise #3503

Closed
jussik opened this issue Aug 8, 2013 · 10 comments
Closed

$watching functions which return a deferred/promise #3503

jussik opened this issue Aug 8, 2013 · 10 comments

Comments

@jussik
Copy link
Contributor

jussik commented Aug 8, 2013

If I $watch a promise it will treat what the promise is resolved with as the value to watch, which is good. If, however, I $watch a function returns that same promise, $digest handles the promise like a plain object which causes the watch not to trigger on resolve, and anything relying on the watch for the output value (e.g. ng-bind) will receive the promise object instead of the value it was resolved with.

Fiddle (outputs to console)

The check for deferred/promise is in getterFn and that's never used for function calls, should there be a check to see if the value of the expression is a deferred/promise in $digest?

@guillaume86
Copy link

I think it should have been handled in $watch and not in $parse.
How can I handle a function call that return a new promise now ("match in search($viewModel)") ?
With $parse it will return undefined, with $watch it will get a new promise everytime...

See: angular-ui/bootstrap#949 for a common case

@jussik
Copy link
Contributor Author

jussik commented Sep 11, 2013

Promise objects in scope are handled in $parse so it makes sense to handle functions returning promises there as well, otherwise you'd get inconsistent functionality in places. If it was implemented in $watch it would work in ngBind but not ngRepeat, for example.

I can't think of anything universal enough other than just having some token in the promise which tells $parse not to unwrap it automatically, but that feels too kludgy and would need support from everything that generates promises. Whatever the methodology is, it would really need to work identically with plain promise objects in scope to retain consistency.

Do you have a trivial use case (applicable in the real world) that doesn't use any third party libraries? I don't use typeahead so drudging through its sources is feeling a bit unfruitful.

@pkozlowski-opensource
Copy link
Member

@jussik I'm the one working on the typeahead directive. Basically this commit breaks possibility to have a syntax like the one used in the select directive in any custom directive, if we want to handle async stuff. So we might argue that it is not a big deal since the core is not impacted. But it makes the $parse service pretty much useless for many use-cases. What is worse for me is that the $parse interface looks like synchronous one, but in fact behave like async one in some cases. This is all good if used only from the view but super-confusing for any other use-case.

What I would like to propose is to have 2 services: one $parse (or $rawParse or whatever) that doesn't do any promise unwrapping plus a decorator around it that does promise unwrapping and can be used from view-related directives. @IgorMinar @vojtajina I'm not too familiar with the $parse internals but this commit is breaking very useful service pretty badly. Could we have a chat about it? My main issue for now is that when $parse returns undefined I've got no way of knowing if the expression really evaluates to undefined or is it evaluating to the async result.

@jussik
Copy link
Contributor Author

jussik commented Sep 11, 2013

If a $parseRaw (or similar) would be adequate then the fix could be contained within $parse, which is always good.

I see where you're coming from with $parse's behaviour being inconsistent, however it mimics the functionality of how $parse handles promise objects in scope, which is a large part of what makes ngResource work as far as I can tell. In the end removing the fix outright and retaining previous functionality would add inconsistency rather than reducing it in my eyes.

The issue needs a fix of course and I'll help however I can.

@pkozlowski-opensource
Copy link
Member

@jussik I hear what you are saying. I know that it was inconsistent before your commit. What I'm trying to say here is that currently $parse starts to do 2 slightly different things which limits its usefulness in the non-template context. On top of this it seem to have a bit of code duplication when it comes to promise-unwrapping code.

This is something I need to discuss with the rest of the AngularJS team.

@xrg
Copy link

xrg commented Sep 12, 2013

I agree with @pkoslowski . After this patch, $parse() will always return undefined, thus ignoring the result of the promise, which is not what a developer would expect.
If you don't like promises in $parse, just throw an exception instead.

@wilsonjackson
Copy link

Here's a much simpler 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. 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.

@petebacondarwin
Copy link
Member

Can someone open a new issue to get this change looked at/reverted? Thanks

@wilsonjackson
Copy link

Submitted #4158

@andreas-gruenbacher
Copy link
Contributor

Note that this commit has broken asynchronous validation in ui-validate (see angular-ui/ui-utils#120).

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.

7 participants