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

Commit

Permalink
fix($parse): handle promises returned from parsed function calls
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jussik authored and vojtajina committed Aug 15, 2013
1 parent 37123cd commit 3a65822
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
14 changes: 13 additions & 1 deletion src/ng/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -689,9 +689,21 @@ function parser(text, json, $filter, csp){
}
var fnPtr = fn(scope, locals, context) || noop;
// IE stupidity!
return fnPtr.apply
var v = fnPtr.apply
? fnPtr.apply(context, args)
: fnPtr(args[0], args[1], args[2], args[3], args[4]);

// Check for promise
if (v && v.then) {
var p = v;

This comment has been minimized.

Copy link
@xrg

xrg Sep 12, 2013

What is the point in copying here?

This comment has been minimized.

Copy link
@jussik

jussik Sep 29, 2013

Author Contributor

v is what we return later on in the function, p needs to be a separate reference (note: it's not a deep copy) as we use it in the .then callback which is likely executed after we've returned v. You're right in that we don't need it yet, but we will.

if (!('$$v' in v)) {
p.$$v = undefined;
p.then(function(val) { p.$$v = val; });
}
v = v.$$v;

This comment has been minimized.

Copy link
@xrg

xrg Sep 12, 2013

so, by deep copying the current value, "v" becomes undefined. And the promise "v" aka "p" is just discarded, isn't it?

This comment has been minimized.

Copy link
@jussik

jussik Sep 29, 2013

Author Contributor

It's not a deep copy, it's only a reference assignment. v is only undefined until our promise resolves, after which it gets assigned the value the promise was resolved with (see the .then callback). As for p being discarded, see my previous comment, it's used in the .then callback to assign $$v.

This comment has been minimized.

Copy link
@xrg

xrg Sep 30, 2013

Please correct me if I'm wrong: since undefined is a primitive, isn't that copied by value? And, if you are unwrapping the value out of the object, assigning a new value as attribute to that non-exposed object, means that the value will not be accessible? Or does Javascript work in another way (I'm new to this..) ?

I'm not talking about subsequent calls to the fn, where the promise is resolved before v = v.$$v is evaluated.

This comment has been minimized.

Copy link
@jussik

jussik Sep 30, 2013

Author Contributor

Correct, undefined is copied by value, although nothing is really copied here as you're assigning a primitive literal. The unwrapping simply changes where v is pointing, so any other references to the promise (in the code that will be resolving it as well as p that we defined above) will continue to point to to the entire promise rather than $$v.

If v was the only reference to the promise then it might be eligible for garbage collection, although I don't know if you can garbage collect an object if there's a reference to one of its members.

This comment has been minimized.

Copy link
@xrg

xrg Sep 30, 2013

.. which comes to my big objection about this whole patch: if the result of parsing is the only reference to a unique promise, then that promise will never become accessible outside the $parse() call.
So, we will have to write non-intuitive code to work around that limitation. 👎
IMHO, the "expected" behavior would be to keep the promise until it can be resolved, somehow.

This comment has been minimized.

Copy link
@jussik

jussik Sep 30, 2013

Author Contributor

This is being discussed in #4158. I agree that it's limiting not to be able to access the promise and it seems like $parse will likely support an option for it to work either way, but since promise objects in scope are unwrapped automatically I would say the expected behaviour would be that functions returning promises work the same way.

}

return v;
};
}

Expand Down
12 changes: 12 additions & 0 deletions test/ng/parseSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,18 @@ describe('parser', function() {
expect(scope.$eval('greeting')).toBe(undefined);
});

it('should evaluate a function call returning a promise and eventually get its return value', function() {
scope.greetingFn = function() { return promise; };
expect(scope.$eval('greetingFn()')).toBe(undefined);

This comment has been minimized.

Copy link
@xrg

xrg Sep 12, 2013

Aren't these /different/ calls of greetingFn() ?
What if they were async calls to some server, for example?
IMHO we are testing for a behavior different than expected.

This comment has been minimized.

Copy link
@jussik

jussik Sep 29, 2013

Author Contributor

In this case what's I'm after is what $parse does to a promise at different phases of resolution. greetingFn() always returns a reference to the same promise.

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Sep 29, 2013

Member

@jussik - what happens if this is not the case - I mean, if a function returns different promises when called at different times?

This comment has been minimized.

Copy link
@jussik

jussik Sep 29, 2013

Author Contributor

Then it starts a separate instance of the process being tested here. You're right in that I don't test multiple concurrent promises but since the return value is contained within the promise object there shouldn't be any crosstalk between separate promises. I know "shouldn't" isn't a justification when it comes to test code, I just didn't consider writing a test for it, although I probably should have.


scope.$digest();
expect(scope.$eval('greetingFn()')).toBe(undefined);

deferred.resolve('hello!');
expect(scope.$eval('greetingFn()')).toBe(undefined);
scope.$digest();
expect(scope.$eval('greetingFn()')).toBe('hello!');
});

describe('assignment into promises', function() {
// This behavior is analogous to assignments to non-promise values
Expand Down

6 comments on commit 3a65822

@dennis-g
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this I'm getting an error in Angular-UI Typeahead. When calling "parserResult.source(scope, locals)" it returns "undefined".
So, maybe there is something wrong in your commit?

https://github.com/angular-ui/bootstrap/blob/master/src/typeahead/typeahead.js#L99

@chrisronline
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a fiddle of the broken state in 1.1.5: http://jsfiddle.net/Y6Myh/1/

I just found this today and glad to see it's been fixed!

@guillaume86
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That change made $parse pretty useless when using promises (forced to use $watch now).
See angular-ui/bootstrap#949 for an exemple.

@wilsonjackson
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes a pretty big assumption about how promises returned by expressions are intended to be used. Even if it suits the majority case, we need some way to parse an expression and get the raw promise back. Are there any plans to provide an alternate service that doesn't do magical unwrapping?

@petebacondarwin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonjackson - Perhaps you could open a new issue to discuss this?

@wilsonjackson
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.