fix($parse) - Allow setterFn to bind correctly to promise results #2854
Conversation
When binding the result of a promise directly to an ng:model directive, getterFn successfully extracts the data from the promise by traversing the '$$v' attribute. However the setterFn was not successfully traversing to the $$v attribute, and instead writing model updates directly on the promise itself. The subsequent apply/digest phase of Angular was then pulling the original value from the promise and applying it back into the viewState, thus simulating a read-only field. In summary, setterFn was writing to the root of the promise, while getterFn was reading from the '$$v' component. Fixed by modifying setterFn to unwrap promises if found and read $$v. If promises have not been resolved yet, bindings will function as they previously did (act as a read-only field). This appears to be more of a side effect than intentional design in the existing codebase, however I kept this behaviour in this patch to minimize the chance of breaking existing projects. Closes angular#1827 Also see: http://stackoverflow.com/questions/16883239/angularjs-ngmodel- field-is-readonly-when-bound-to-q-promise/16883387
| function unwrapPromise(obj){ | ||
|
|
||
| // If we'v been passed a promise | ||
| if( obj.then ){ |
There was a problem hiding this comment.
Happy to adjust. Can you elaborate on the issue?
There was a problem hiding this comment.
You seem to be using a different style than the rest of this file (if (is('(){}[].,;:?')) vs if( obj.then ){ for example)
There was a problem hiding this comment.
Ta. I've gone with the style
if(condition) {
// Block
}
which appears to be commonly used throughout that file?
I was missing a null check (which I've just added), and have expanded on my function comments a bit more. Closer to what's expected? (The if() statement is now identical to other promise check within the same file)
There was a problem hiding this comment.
Oh, right. That's obvious now that I see it. Will adjust.
|
we are experiencing this bug in our app would be nice if it could be merged into the stable branch |
|
+1 as someone relatively new to Angular this took me a day to track down and workaround, surprised more people haven't run into it. |
|
+1 |
1 similar comment
|
+1 |
|
-1 - personally I don't like the idea of auto-unwrapping promises. It hides what is really going on. |
|
This is a feature not a fix... PR Checklist (Minor Feature)
|
|
@zoomzoom83 - can you sign the CLA? |
|
@petebacondarwin CLA Signed - "James Davies" |
At the moment, AngularJS appears to unwrap promises automatically in all other scenarios - from an end-developers perspective, this comes across as a bug when contrasted with other behaviour. I'm not sure if this particular scenario was intentional or not, but it doesn't appear to be. I'm aware this has been discussed on the forums and the existing promise unwrapping functionality has been considered for removal - but in the meantime it makes sense to add this in for consistency. Going in the opposite direction would break existing apps. (My vote would be to keep this functionality - I'm making very heavy use of it, and it dramatically simplifies the code and removes a lot of bug-prone boilerplate. There's minimal confusion as to what's happening). |
|
We're going to discuss this feature as a team. There are three possible outcomes:
|
|
Landed as 61906d3 |
When binding the result of a promise directly to an ng:model directive,
getterFn successfully extracts the data from the promise by traversing
the '$$v' attribute.
However the setterFn was not successfully traversing to the $$v
attribute, and instead writing model updates directly on the promise
itself.
The subsequent apply/digest phase of Angular was then pulling the
original value from the promise and applying it back into the viewState,
thus simulating a read-only field.
In summary, setterFn was writing to the root of the promise, while
getterFn was reading from the '$$v' component.
Fixed by modifying setterFn to unwrap promises if found and read $$v.
If promises have not been resolved yet, bindings will function as they
previously did (act as a read-only field). This appears to be more of a
side effect than intentional design in the existing codebase, however
I kept this behaviour in this patch to minimize the chance of breaking
existing projects.
Closes #1827
This issue affects the 1.0.x branch as well and would be worth backporting.
(Cleanly cherry-picks against 1.0.7 with all tests passing).
Also see: http://stackoverflow.com/questions/16883239/angularjs-ngmodel-
field-is-readonly-when-bound-to-q-promise/16883387