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

Commit

Permalink
fix($rootScope): Set no context when calling helper functions for $watch
Browse files Browse the repository at this point in the history 
When calling a $watch getter or listener, do not expose the inner workings with `this`.

Closes: #13909
  • Loading branch information
@lgalfaso
lgalfaso committed Feb 1, 2016
1 parent 8bda5ec commit 1c6edd4
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 3 deletions.
8 changes: 5 additions & 3 deletions src/ng/rootScope.js
View file
Expand Up @@ -744,7 +744,7 @@ function $RootScopeProvider() {
*
*/
$digest: function() {
var watch, value, last,
var watch, value, last, fn, get,
watchers,
length,
dirty, ttl = TTL,
Expand Down Expand Up @@ -790,15 +790,17 @@ function $RootScopeProvider() {
// Most common watches are on primitives, in which case we can short
// circuit it with === operator, only when === fails do we use .equals
if (watch) {
if ((value = watch.get(current)) !== (last = watch.last) &&
get = watch.get;
if ((value = get(current)) !== (last = watch.last) &&
!(watch.eq
? equals(value, last)
: (typeof value === 'number' && typeof last === 'number'
&& isNaN(value) && isNaN(last)))) {
dirty = true;
lastDirtyWatch = watch;
watch.last = watch.eq ? copy(value, null) : value;
watch.fn(value, ((last === initWatchVal) ? value : last), current);
fn = watch.fn;
fn(value, ((last === initWatchVal) ? value : last), current);
if (ttl < 5) {
logIdx = 4 - ttl;
if (!watchLog[logIdx]) watchLog[logIdx] = [];
Expand Down
14 changes: 14 additions & 0 deletions test/ng/rootScopeSpec.js
View file
Expand Up @@ -117,6 +117,20 @@ describe('Scope', function() {
}));


it('should not expose the `inner working of watch', inject(function($rootScope) {
function Getter() {
expect(this).toBeUndefined();
return 'foo';
}
function Listener() {
expect(this).toBeUndefined();
}
if (msie < 10) return;
$rootScope.$watch(Getter, Listener);
$rootScope.$digest();
}));


it('should watch and fire on expression change', inject(function($rootScope) {
var spy = jasmine.createSpy();
$rootScope.$watch('name.first', spy);
Expand Down

6 comments on commit 1c6edd4

@guziakas
Copy link

Choose a reason for hiding this comment

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

Sadly this was removed, now how i can get the watchExpression this.exp inside listener function?

@lgalfaso
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guziakas would it be possible to know what the use case for this is?

@guziakas
Copy link

Choose a reason for hiding this comment

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

I have written directive where you can specify filter in attribute, you provide a list of fields to watch and react on value changes,
code sample:

if ("valueFilter" in attrs) {

    var filters = scope.$eval(attrs.valueFilter);
    $.each(filters, function (i, v) {
        var filterField = v;
        // watches are set dinamicaly
        scope.$watch(v.scopeField, function (value, oldValue) {
            console.log("filter has changed on parent, update filter on control");
            var scopeFieldName = this.exp;
            // set filter value
            setFilter(scopeFieldName, value);
        });
    });
}

@lgalfaso
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guziakas why not

if ("valueFilter" in attrs) {
  var filters = scope.$eval(attrs.valueFilter);
  $.each(filters, function (i, v) {
    var filterField = v;
    // watches are set dynamically
    scope.$watch(v.scopeField, function (value, oldValue) {
      console.log("filter has changed on parent, update filter on control");
      var scopeFieldName = v.scopeField;
      // set filter value
      setFilter(scopeFieldName, value);
    });
  });
}

We were exposing the inner-workings on how watch works, and this is not part of the public API. Sorry, but the context will not be restored.

@guziakas
Copy link

Choose a reason for hiding this comment

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

That approach works perfectly! Thanks for good and simple solution :)

@derekm
Copy link

@derekm derekm commented on 1c6edd4 Mar 2, 2016

Choose a reason for hiding this comment

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

I've written a <chart>'ing C3/D3 wrapper directive that dynamically instantiates chart-type implementations based on a chart's options. Thanks to this fix I can do var ChartImpl = this[type.charAt(0).toUpperCase() + type.substr(1) + "ChartImpl"]; var chart = new ChartImpl(data, options);.

My implementation is not backwards compatible to prior releases of Angular because of the screwed up this context in those releases.

Please ensure that this here and elsewhere remains the global context so we can utilize the full power of non-strict JavaScript! Thanks!

Please sign in to comment.