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

fix($parse): standardize one-time literal vs non-literal and interceptors #15858

Merged
merged 3 commits into from
Mar 31, 2017
Merged

fix($parse): standardize one-time literal vs non-literal and interceptors #15858

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a58a052
fix($parse): standardize one-time literal vs non-literal and intercep…
jbedard Mar 24, 2017
2087b25
fixup! fix($parse): standardize one-time literal vs non-literal and i…
jbedard Mar 30, 2017
2b6c11b
update variable name
Narretz Mar 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 8 additions & 45 deletions src/ng/directive/ngClass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,6 @@ function classDirective(name, selector) {
return {
restrict: 'AC',
link: function(scope, element, attr) {
var expression = attr[name].trim();
var isOneTime = (expression.charAt(0) === ':') && (expression.charAt(1) === ':');

var watchInterceptor = isOneTime ? toFlatValue : toClassString;
var watchExpression = $parse(expression, watchInterceptor);
var watchAction = isOneTime ? ngClassOneTimeWatchAction : ngClassWatchAction;

var classCounts = element.data('$classCounts');
var oldModulo = true;
var oldClassString;
Expand All @@ -43,7 +36,7 @@ function classDirective(name, selector) {
scope.$watch(indexWatchExpression, ngClassIndexWatchAction);
}

scope.$watch(watchExpression, watchAction, isOneTime);
scope.$watch($parse(attr[name], toClassString), ngClassWatchAction);

function addClasses(classString) {
classString = digestClassCounts(split(classString), 1);
Expand Down Expand Up @@ -85,9 +78,9 @@ function classDirective(name, selector) {
}

function ngClassIndexWatchAction(newModulo) {
// This watch-action should run before the `ngClass[OneTime]WatchAction()`, thus it
// This watch-action should run before the `ngClassWatchAction()`, thus it
// adds/removes `oldClassString`. If the `ngClass` expression has changed as well, the
// `ngClass[OneTime]WatchAction()` will update the classes.
// `ngClassWatchAction()` will update the classes.
if (newModulo === selector) {
addClasses(oldClassString);
} else {
Expand All @@ -97,15 +90,13 @@ function classDirective(name, selector) {
oldModulo = newModulo;
}

function ngClassOneTimeWatchAction(newClassValue) {
var newClassString = toClassString(newClassValue);

if (newClassString !== oldClassString) {
ngClassWatchAction(newClassString);
function ngClassWatchAction(newClassString) {
//When using a one-time binding the newClassString will return
Copy link
Member

Choose a reason for hiding this comment

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

You realy don't like spaces after //, do you 😛

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bad (or just different!) habit I guess :)

//the pre-interceptor value until the one-time is complete
if (newClassString && typeof newClassString !== "string") {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just !isString(newClassString)?
Also, I don't think it makes much difference over always applying toClassString() (which just does isArray() and isObject()).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated to isString. I kind of like an if block just so it's obvious that the comment applies to the full block, nothing more nothing less...

newClassString = toClassString(newClassString);
}
}

function ngClassWatchAction(newClassString) {
if (oldModulo === selector) {
updateClasses(oldClassString, newClassString);
}
Expand Down Expand Up @@ -152,34 +143,6 @@ function classDirective(name, selector) {

return classString;
}

function toFlatValue(classValue) {
var flatValue = classValue;

if (isArray(classValue)) {
flatValue = classValue.map(toFlatValue);
} else if (isObject(classValue)) {
var hasUndefined = false;

flatValue = Object.keys(classValue).filter(function(key) {
var value = classValue[key];

if (!hasUndefined && isUndefined(value)) {
hasUndefined = true;
}

return value;
});

if (hasUndefined) {
// Prevent the `oneTimeLiteralWatchInterceptor` from unregistering
// the watcher, by including at least one `undefined` value.
flatValue.push(undefined);
}
}

return flatValue;
}
}

/**
Expand Down
64 changes: 26 additions & 38 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1765,8 +1765,8 @@ function $ParseProvider() {
if (parsedExpression.constant) {
parsedExpression.$$watchDelegate = constantWatchDelegate;
} else if (oneTime) {
parsedExpression.$$watchDelegate = parsedExpression.literal ?
oneTimeLiteralWatchDelegate : oneTimeWatchDelegate;
parsedExpression.oneTime = true;
parsedExpression.$$watchDelegate = oneTimeWatchDelegate;
} else if (parsedExpression.inputs) {
parsedExpression.$$watchDelegate = inputsWatchDelegate;
}
Expand Down Expand Up @@ -1852,6 +1852,7 @@ function $ParseProvider() {
}

function oneTimeWatchDelegate(scope, listener, objectEquality, parsedExpression, prettyPrintExpression) {
var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could have a better name, e.g. isDone, fulfillsDoneCriteria, because it's not very descriptive when called later on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, done.

var unwatch, lastValue;
if (parsedExpression.inputs) {
unwatch = inputsWatchDelegate(scope, oneTimeListener, objectEquality, parsedExpression, prettyPrintExpression);
Expand All @@ -1868,41 +1869,22 @@ function $ParseProvider() {
if (isFunction(listener)) {
listener(value, old, scope);
}
if (isDefined(value)) {
if (doneCriteria(value)) {
scope.$$postDigest(function() {
if (isDefined(lastValue)) {
if (doneCriteria(lastValue)) {
unwatch();
}
});
}
}
}

function oneTimeLiteralWatchDelegate(scope, listener, objectEquality, parsedExpression) {
var unwatch, lastValue;
unwatch = scope.$watch(function oneTimeWatch(scope) {
return parsedExpression(scope);
}, function oneTimeListener(value, old, scope) {
lastValue = value;
if (isFunction(listener)) {
listener(value, old, scope);
}
if (isAllDefined(value)) {
scope.$$postDigest(function() {
if (isAllDefined(lastValue)) unwatch();
});
}
}, objectEquality);

return unwatch;

function isAllDefined(value) {
var allDefined = true;
forEach(value, function(val) {
if (!isDefined(val)) allDefined = false;
});
return allDefined;
}
function isAllDefined(value) {
var allDefined = true;
forEach(value, function(val) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Not directly related to this PR, but I think it makes sense to exit early once an undefined value is found.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think value can be an object or array which forEach currently handles or us. Did you have something in mind?

Since this is currently only used for object/array literal cases, we could split this into separate object/array helper methods (one using for-in one using array.every?)...

Copy link
Member

Choose a reason for hiding this comment

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

I agree it is not worth it. It is just for one-time literals and it will only matter if one has huge literals which don't settle quickly (a very unlikely usecase).

if (!isDefined(val)) allDefined = false;
});
return allDefined;
}

function constantWatchDelegate(scope, listener, objectEquality, parsedExpression) {
Expand All @@ -1918,22 +1900,28 @@ function $ParseProvider() {
var watchDelegate = parsedExpression.$$watchDelegate;
var useInputs = false;

var regularWatch =
watchDelegate !== oneTimeLiteralWatchDelegate &&
watchDelegate !== oneTimeWatchDelegate;
var doneCriteria = parsedExpression.literal ? isAllDefined : isDefined;

var fn = regularWatch ? function regularInterceptedExpression(scope, locals, assign, inputs) {
function regularInterceptedExpression(scope, locals, assign, inputs) {
var value = useInputs && inputs ? inputs[0] : parsedExpression(scope, locals, assign, inputs);
return interceptorFn(value, scope, locals);
} : function oneTimeInterceptedExpression(scope, locals, assign, inputs) {
var value = parsedExpression(scope, locals, assign, inputs);
}

function oneTimeInterceptedExpression(scope, locals, assign, inputs) {
var value = useInputs && inputs ? inputs[0] : parsedExpression(scope, locals, assign, inputs);
var result = interceptorFn(value, scope, locals);
// we only return the interceptor's result if the
// initial value is defined (for bind-once)
return isDefined(value) ? result : value;
};
return doneCriteria(value) ? result : value;
}

var fn = parsedExpression.oneTime ? oneTimeInterceptedExpression : regularInterceptedExpression;

// Propogate the literal/oneTime attributes
fn.literal = parsedExpression.literal;
fn.oneTime = parsedExpression.oneTime;

// Propagate $$watchDelegates other then inputsWatchDelegate
// Propagate or create inputs / $$watchDelegates
useInputs = !parsedExpression.inputs;
if (watchDelegate && watchDelegate !== inputsWatchDelegate) {
fn.$$watchDelegate = watchDelegate;
Expand Down
156 changes: 80 additions & 76 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2688,82 +2688,86 @@ describe('parser', function() {
expect($parse(':: ').literal).toBe(true);
}));

it('should only become stable when all the properties of an object have defined values', inject(function($parse, $rootScope, log) {
var fn = $parse('::{foo: foo, bar: bar}');
$rootScope.$watch(fn, function(value) { log(value); }, true);

expect(log.empty()).toEqual([]);
expect($rootScope.$$watchers.length).toBe(1);

$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([{foo: undefined, bar: undefined}]);

$rootScope.foo = 'foo';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([{foo: 'foo', bar: undefined}]);

$rootScope.foo = 'foobar';
$rootScope.bar = 'bar';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([{foo: 'foobar', bar: 'bar'}]);

$rootScope.foo = 'baz';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([]);
}));

it('should only become stable when all the elements of an array have defined values', inject(function($parse, $rootScope, log) {
var fn = $parse('::[foo,bar]');
$rootScope.$watch(fn, function(value) { log(value); }, true);

expect(log.empty()).toEqual([]);
expect($rootScope.$$watchers.length).toBe(1);

$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([[undefined, undefined]]);

$rootScope.foo = 'foo';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([['foo', undefined]]);

$rootScope.foo = 'foobar';
$rootScope.bar = 'bar';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([['foobar', 'bar']]);

$rootScope.foo = 'baz';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([]);
}));

it('should only become stable when all the elements of an array have defined values at the end of a $digest', inject(function($parse, $rootScope, log) {
var fn = $parse('::[foo]');
$rootScope.$watch(fn, function(value) { log(value); }, true);
$rootScope.$watch('foo', function() { if ($rootScope.foo === 'bar') {$rootScope.foo = undefined; } });

$rootScope.foo = 'bar';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(2);
expect(log.empty()).toEqual([['bar'], [undefined]]);

$rootScope.foo = 'baz';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([['baz']]);

$rootScope.bar = 'qux';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log).toEqual([]);
}));
[true, false].forEach(function(isDeep) {
describe(isDeep ? 'deepWatch' : 'watch', function() {
it('should only become stable when all the properties of an object have defined values', inject(function($parse, $rootScope, log) {
var fn = $parse('::{foo: foo, bar: bar}');
$rootScope.$watch(fn, function(value) { log(value); }, isDeep);

expect(log.empty()).toEqual([]);
expect($rootScope.$$watchers.length).toBe(1);

$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([{foo: undefined, bar: undefined}]);

$rootScope.foo = 'foo';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([{foo: 'foo', bar: undefined}]);

$rootScope.foo = 'foobar';
$rootScope.bar = 'bar';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([{foo: 'foobar', bar: 'bar'}]);

$rootScope.foo = 'baz';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([]);
}));

it('should only become stable when all the elements of an array have defined values', inject(function($parse, $rootScope, log) {
var fn = $parse('::[foo,bar]');
$rootScope.$watch(fn, function(value) { log(value); }, isDeep);

expect(log.empty()).toEqual([]);
expect($rootScope.$$watchers.length).toBe(1);

$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([[undefined, undefined]]);

$rootScope.foo = 'foo';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([['foo', undefined]]);

$rootScope.foo = 'foobar';
$rootScope.bar = 'bar';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([['foobar', 'bar']]);

$rootScope.foo = 'baz';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(0);
expect(log.empty()).toEqual([]);
}));

it('should only become stable when all the elements of an array have defined values at the end of a $digest', inject(function($parse, $rootScope, log) {
var fn = $parse('::[foo]');
$rootScope.$watch(fn, function(value) { log(value); }, isDeep);
$rootScope.$watch('foo', function() { if ($rootScope.foo === 'bar') {$rootScope.foo = undefined; } });

$rootScope.foo = 'bar';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(2);
expect(log.empty()).toEqual([['bar'], [undefined]]);

$rootScope.foo = 'baz';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log.empty()).toEqual([['baz']]);

$rootScope.bar = 'qux';
$rootScope.$digest();
expect($rootScope.$$watchers.length).toBe(1);
expect(log).toEqual([]);
}));
});
});
});
});

Expand Down