refactor($rootScope): asyncQueue is pre-$parsed, no need to call $eval #15682
Conversation
src/ng/rootScope.js
Outdated
| // lead to a '$digest already in progress' error. | ||
| for (var asyncQueuePosition = 0; asyncQueuePosition < asyncQueue.length; asyncQueuePosition++) { | ||
| try { | ||
| asyncTask = asyncQueue[asyncQueuePosition]; | ||
| asyncTask.scope.$eval(asyncTask.expression, asyncTask.locals); | ||
| asyncTask.expression(asyncTask.scope, asyncTask.locals); |
There was a problem hiding this comment.
Won't this affect the this inside expression?
There was a problem hiding this comment.
If you mean string expressions like 'this.foo', no, it won't affect them:
angular.injector(['ng']).get('$parse')('this.foo').call({foo: 1}, {foo: 2}); // 2If you mean functions passed as expressions to $evalAsync, it will affect them, but does this matter?
There was a problem hiding this comment.
Yes, I mean functions. I think it does (matter). For example, having a function that incorrectly tries to access this.someProp, would previously throw (because this === undefined), but will now silently fail.
I would rather not have the current behavior changed. (I mean would not change "this being undefined" - skipping the extra parsing (even if a no-op) lgtm.)
thorn0
force-pushed
the
asyncQueue
branch
2 times, most recently
from
adf5752 to
6393d6a
Compare
|
@gkalpak ping |
| // lead to a '$digest already in progress' error. | ||
| for (var asyncQueuePosition = 0; asyncQueuePosition < asyncQueue.length; asyncQueuePosition++) { | ||
| try { | ||
| asyncTask = asyncQueue[asyncQueuePosition]; | ||
| asyncTask.scope.$eval(asyncTask.expression, asyncTask.locals); | ||
| fn = asyncTask.fn; |
There was a problem hiding this comment.
As @jbedard correctly pointed out, it is a good idea to have some test about the context (or lack thereof), to guard against future "optimizations".
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
minor refactoring
Does this PR introduce a breaking change?
no