Skip to content

Commit

Permalink
fix($parse): make promise unwrapping opt-in
Browse files Browse the repository at this point in the history 
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 angular#4158
Closes angular#4270
  • Loading branch information
@IgorMinar
IgorMinar committed Oct 7, 2013
1 parent 49e06ea commit 978d4a3
Show file tree
Hide file tree
Showing 2 changed files with 306 additions and 158 deletions.
162 changes: 121 additions & 41 deletions src/ng/parse.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,16 @@ var ESCAPE = {"n":"\n", "f":"\f", "r":"\r", "t":"\t", "v":"\v", "'":"'", '"':'"'
/**
* @constructor
*/
var Lexer = function (csp) {
this.csp = csp;
var Lexer = function (options) {
this.options = options;
};

Lexer.prototype = {
constructor: Lexer,

lex: function (text) {
this.text = text;

this.index = 0;
this.ch = undefined;
this.lastCh = ':'; // can start regexp
Expand Down Expand Up @@ -294,12 +295,12 @@ Lexer.prototype = {
token.fn = OPERATORS[ident];
token.json = OPERATORS[ident];
} else {
var getter = getterFn(ident, this.csp, this.text);
var getter = getterFn(ident, this.options, this.text);
token.fn = extend(function(self, locals) {
return (getter(self, locals));
}, {
assign: function(self, value) {
return setter(self, ident, value, parser.text);
return setter(self, ident, value, parser.text, parser.options);
}
});
}
Expand Down Expand Up @@ -370,10 +371,10 @@ Lexer.prototype = {
/**
* @constructor
*/
var Parser = function (lexer, $filter, csp) {
var Parser = function (lexer, $filter, options) {
this.lexer = lexer;
this.$filter = $filter;
this.csp = csp;
this.options = options;
};

Parser.ZERO = function () { return 0; };
Expand All @@ -387,7 +388,7 @@ Parser.prototype = {
//TODO(i): strip all the obsolte json stuff from this file
this.json = json;

this.tokens = this.lexer.lex(text, this.csp);
this.tokens = this.lexer.lex(text);

if (json) {
// The extra level of aliasing is here, just in case the lexer misses something, so that
Expand Down Expand Up @@ -687,13 +688,13 @@ Parser.prototype = {
fieldAccess: function(object) {
var parser = this;
var field = this.expect().text;
var getter = getterFn(field, this.csp, this.text);
var getter = getterFn(field, this.options, this.text);

return extend(function(scope, locals, self) {
return getter(self || object(scope, locals), locals);
}, {
assign: function(scope, value, locals) {
return setter(object(scope, locals), field, value, parser.text);
return setter(object(scope, locals), field, value, parser.text, parser.options);
}
});
},
Expand All @@ -711,7 +712,7 @@ Parser.prototype = {

if (!o) return undefined;
v = ensureSafeObject(o[i], parser.text);
if (v && v.then) {
if (v && v.then && parser.options.unwrapPromises) {
p = v;
if (!('$$v' in v)) {
p.$$v = undefined;
Expand Down Expand Up @@ -758,7 +759,7 @@ Parser.prototype = {
: fnPtr(args[0], args[1], args[2], args[3], args[4]);

// Check for promise
if (v && v.then) {
if (v && v.then && parser.options.unwrapPromises) {
var p = v;
if (!('$$v' in v)) {
p.$$v = undefined;
Expand Down Expand Up @@ -826,15 +827,17 @@ Parser.prototype = {
literal: true,
constant: allConstant
});
},
}
};


//////////////////////////////////////////////////
// Parser helper functions
//////////////////////////////////////////////////

function setter(obj, path, setValue, fullExp) {
function setter(obj, path, setValue, fullExp, options) {
options = options || {};

var element = path.split('.'), key;
for (var i = 0; element.length > 1; i++) {
key = ensureSafeMemberName(element.shift(), fullExp);
Expand All @@ -844,7 +847,7 @@ function setter(obj, path, setValue, fullExp) {
obj[key] = propertyObj;
}
obj = propertyObj;
if (obj.then) {
if (obj.then && options.unwrapPromises) {
if (!("$$v" in obj)) {
(function(promise) {
promise.then(function(val) { promise.$$v = val; }); }
Expand All @@ -868,7 +871,7 @@ var getterFnCache = {};
* - http://jsperf.com/angularjs-parse-getter/4
* - http://jsperf.com/path-evaluation-simplified/7
*/
function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp, options) {
ensureSafeMemberName(key0, fullExp);
ensureSafeMemberName(key1, fullExp);
ensureSafeMemberName(key2, fullExp);
Expand All @@ -881,7 +884,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
if (pathVal === null || pathVal === undefined) return pathVal;

pathVal = pathVal[key0];
if (pathVal && pathVal.then) {
if (pathVal && pathVal.then && options.unwrapPromises) {
if (!("$$v" in pathVal)) {
promise = pathVal;
promise.$$v = undefined;
Expand All @@ -892,7 +895,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
if (!key1 || pathVal === null || pathVal === undefined) return pathVal;

pathVal = pathVal[key1];
if (pathVal && pathVal.then) {
if (pathVal && pathVal.then && options.unwrapPromises) {
if (!("$$v" in pathVal)) {
promise = pathVal;
promise.$$v = undefined;
Expand All @@ -903,7 +906,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
if (!key2 || pathVal === null || pathVal === undefined) return pathVal;

pathVal = pathVal[key2];
if (pathVal && pathVal.then) {
if (pathVal && pathVal.then && options.unwrapPromises) {
if (!("$$v" in pathVal)) {
promise = pathVal;
promise.$$v = undefined;
Expand All @@ -914,7 +917,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
if (!key3 || pathVal === null || pathVal === undefined) return pathVal;

pathVal = pathVal[key3];
if (pathVal && pathVal.then) {
if (pathVal && pathVal.then && options.unwrapPromises) {
if (!("$$v" in pathVal)) {
promise = pathVal;
promise.$$v = undefined;
Expand All @@ -925,7 +928,7 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
if (!key4 || pathVal === null || pathVal === undefined) return pathVal;

pathVal = pathVal[key4];
if (pathVal && pathVal.then) {
if (pathVal && pathVal.then && options.unwrapPromises) {
if (!("$$v" in pathVal)) {
promise = pathVal;
promise.$$v = undefined;
Expand All @@ -937,23 +940,25 @@ function cspSafeGetterFn(key0, key1, key2, key3, key4, fullExp) {
};
}

function getterFn(path, csp, fullExp) {
if (getterFnCache.hasOwnProperty(path)) {
return getterFnCache[path];
function getterFn(path, options, fullExp) {
var cacheKey = path;
cacheKey += '#unwrapPromises:' + (!!options.unwrapPromises).toString();
if (getterFnCache.hasOwnProperty(cacheKey)) {
return getterFnCache[cacheKey];
}

var pathKeys = path.split('.'),
pathKeysLength = pathKeys.length,
fn;

if (csp) {
if (options.csp) {
fn = (pathKeysLength < 6)
? cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp)
? cspSafeGetterFn(pathKeys[0], pathKeys[1], pathKeys[2], pathKeys[3], pathKeys[4], fullExp, options)
: function(scope, locals) {
var i = 0, val;
do {
val = cspSafeGetterFn(
pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++], fullExp
pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++], pathKeys[i++], fullExp, options
)(scope, locals);

locals = undefined; // clear after first iteration
Expand All @@ -972,21 +977,23 @@ function getterFn(path, csp, fullExp) {
? 's'
// but if we are first then we check locals first, and if so read it first
: '((k&&k.hasOwnProperty("' + key + '"))?k:s)') + '["' + key + '"]' + ';\n' +
'if (s && s.then) {\n' +
' if (!("$$v" in s)) {\n' +
' p=s;\n' +
' p.$$v = undefined;\n' +
' p.then(function(v) {p.$$v=v;});\n' +
'}\n' +
' s=s.$$v\n' +
'}\n';
(options.unwrapPromises
? 'if (s && s.then) {\n' +
' if (!("$$v" in s)) {\n' +
' p=s;\n' +
' p.$$v = undefined;\n' +
' p.then(function(v) {p.$$v=v;});\n' +
'}\n' +
' s=s.$$v\n' +
'}\n'
: '');
});
code += 'return s;';
fn = Function('s', 'k', code); // s=scope, k=locals
fn.toString = function() { return code; };
}

return getterFnCache[path] = fn;
return getterFnCache[cacheKey] = fn;
}

///////////////////////////////////
Expand Down Expand Up @@ -1030,19 +1037,92 @@ function getterFn(path, csp, fullExp) {
* set to a function to change its value on the given context.
*
*/


/**
* @ngdoc object
* @name ng.$parseProvider
* @function
*
* @description
* `$parseProvider` can be used for configuring the default behavior of the {@link ng.$parse $parse} service.
*/
function $ParseProvider() {
var cache = {};

var defaultOptions = {
csp: false,
unwrapPromises: false
};


/**
* @deprecated Promise unwrapping via $parse is deprecated and will be removed in the future.
*
* @ngdoc method
* @name ng.$parseProvider#unwrapPromises
* @methodOf ng.$parseProvider
* @description
*
* **This feature is deprecated, see deprecation notes below for more info**
*
* If set to true (default is false), $parse will unwrap promises automatically when a promise is found at any part of
* the expression. In other words, if set to true, the expression will always result in a non-promise value.
*
* While the promise is unresolved, it's treated as undefined, but once resolved and fulfilled, the fulfillment value
* is used in place of the promise while evaluating the expression.
*
* # Deprecation notice
*
* 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
*
*
* @param {boolean=} value New value.
* @returns {boolean|self} Returns the current setting when used as getter and self if used as setter.

This comment has been minimized.

Sign in to view

Copy link
@wilsonjackson

wilsonjackson Oct 8, 2013

It looks like this function has no return value when used as a setter, rather than returning self.

This comment has been minimized.

Sign in to view

Copy link
@IgorMinar

IgorMinar Oct 8, 2013

Author Owner

oh, thanks!
*/
this.unwrapPromises = function(value) {
if (isDefined(value)) {
defaultOptions.unwrapPromises = value;
} else {
return defaultOptions.unwrapPromises;
}
};


this.$get = ['$filter', '$sniffer', function($filter, $sniffer) {
return function(exp) {
defaultOptions.csp = $sniffer.csp;

return function(exp, options) {
switch (typeof exp) {
case 'string':
if (cache.hasOwnProperty(exp)) {
return cache[exp];
options = extend({}, defaultOptions, options);

var cacheKey = exp;
forEach(options, function(optionValue, optionName) {
cacheKey += '#' + optionName + ':' + optionValue;
});

if (cache.hasOwnProperty(cacheKey)) {
return cache[cacheKey];
}

var lexer = new Lexer($sniffer.csp);
var parser = new Parser(lexer, $filter, $sniffer.csp);
return cache[exp] = parser.parse(exp, false);
var lexer = new Lexer(options);
var parser = new Parser(lexer, $filter, options);
return cache[cacheKey] = parser.parse(exp, false);

case 'function':
return exp;
Expand Down
Loading

0 comments on commit 978d4a3

Please sign in to comment.