-
Notifications
You must be signed in to change notification settings - Fork 27.5k
$watching functions which return a deferred/promise #3503
Comments
I think it should have been handled in $watch and not in $parse. See: angular-ui/bootstrap#949 for a common case |
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. |
@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 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 |
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. |
@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. |
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. |
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 Because |
Can someone open a new issue to get this change looked at/reverted? Thanks |
Submitted #4158 |
Note that this commit has broken asynchronous validation in ui-validate (see angular-ui/ui-utils#120). |
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?
The text was updated successfully, but these errors were encountered: