fix($timeout/$interval): do not trigger a digest when cancelling a $timeout/$interval #16064

jbedard · 2017-06-21T05:35:48Z

Previously .catch(noop) was used on the rejected timeout/interval to prevent an unhandled rejection error. However this would schedule a deferred task to run the noop. If the cancelling was outside a digest this could cause a new digest such as with the ng-model debounce option.

Fixes #16057

jbedard · 2017-06-21T05:36:23Z

test/ngAnimate/animateCssSpec.js

@@ -1328,8 +1328,7 @@ describe('ngAnimate $animateCss', function() {
          animator.end();

          expect(element.data(ANIMATE_TIMER_KEY)).toBeUndefined();
-          $timeout.flush();


This is an example of how this change can be "breaking" for some tests.

So it's probably not a breaking change? Should we mention this in the review anyway? (But no under Breaking Changes)

@jbedard Can you clarify how this can be "breaking"?

jbedard · 2017-06-21T05:50:02Z

For now I have not removed the 2 promise.catch(noop) calls from resource.js. I think both are already cases where another promise will be resolved so I didn't think it was worth either a) exposing the new helper or b) accessing the internal $$state.pur.

Narretz · 2017-06-23T09:06:52Z

Just saw that the unit tests failed with lots of compile test failures.

Narretz · 2017-06-26T17:24:04Z

src/ng/q.js

@@ -378,8 +378,8 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
    // eslint-disable-next-line no-unmodified-loop-condition
    while (!queueSize && checkQueue.length) {
      var toCheck = checkQueue.shift();
-      if (!toCheck.pur) {
-        toCheck.pur = true;
+      if (isStateExceptionHandled(toCheck)) {


this must be !isStateExceptionHandled(toCheck) and will fix all the failing tests except one

Narretz · 2017-06-26T17:36:35Z

test/ng/directive/ngModelOptionsSpec.js

@@ -459,11 +459,12 @@ describe('ngModelOptions', function() {
          $rootScope.$watch(watchSpy);

          helper.changeInputValueTo('a');
+          expect(function() {$browser.defer.flush();}).toThrowError('No deferred tasks to be flushed');


This doesn't work correctly. It still flushes the task and triggers a digest. I haven't found out why exactly it doesn't work, but I suspect it's because of a mismatch between the real browser and the mock browser.

The assumption that there are no deferred tasks is wrong at this point, because there's obviously a pending timeout.
We just need to check that the watchFn doesn't get called.
If you put in this:

helper.changeInputValueTo('a'); $timeout.flush(2000); helper.changeInputValueTo('b'); $timeout.flush(2000);

the test will fail without the patch. The second timeout.flush is crucial - without it, the watchFn spy doesn't get called, but with it, it gets called twice (without the patch).

jbedard · 2017-06-28T03:24:50Z

Updated. Thanks for pointing out the issues @Narretz

gkalpak · 2017-06-29T13:26:05Z

test/ng/directive/ngModelOptionsSpec.js

@@ -449,7 +449,7 @@ describe('ngModelOptions', function() {
        });


-        it('should not trigger digest while debouncing', function() {
+        it('should not trigger digest while debouncing', inject(function($browser) {


Because it was calling $browser.defer.flush() like the other tests (which is wrong in this case). I'll undo this...

Previously `.catch(noop)` was used on the rejected timeout/interval to prevent an unhandled rejection error. However this would schedule a deferred task to run the `noop`. If the cancelling was outside a digest this could cause a new digest such as with the ng-model `debounce` option. Fixes angular#16057

Previously, `.catch(noop)` was used on a rejected timeout/interval to prevent an unhandled rejection error (introduced in #c9dffde1cb). However this would schedule a deferred task to run the `noop`. If the cancelling was outside a digest this could cause a new digest such as with the ng-model `debounce` option. For unit testing, this means that it's no longer necessary to use `$timeout.flush()` when a `$timeout` has been cancelled outside of a digest. Previously, this was necessary to execute the deferred task added by `.catch(noop). There's an example of such a change in this commit's changeset in the file `/test/ngAnimate/animateCssSpec.js`. Fixes #16057 Closes #16064

abobwhite · 2018-01-26T23:09:04Z

Would this have affected $timeout.cancel from ever returning true in some cases? I have code that used to work fine but now the cancel function always returns false and the timeout is never cancelled.

jbedard · 2018-01-27T00:25:20Z

I don't think this would have changed the return value. Also doesn't look like $browser.defer.cancel changed anytime recently (but I didn't look too hard)...

googlebot added the cla: yes label Jun 21, 2017

jbedard commented Jun 21, 2017

View reviewed changes

jbedard force-pushed the 16057 branch from 5e58ee5 to d557ec1 Compare June 21, 2017 05:37

Nickproger mentioned this pull request Jun 22, 2017

autocomplete(perf): each keypress fires 3 $digests (shift, alt, ctrl key fires infinitely) angular/material#10744

Open

Narretz added this to the 1.6.x milestone Jun 23, 2017

Narretz added severity: broken expected use severity: regression type: bug component: misc core labels Jun 23, 2017

jbedard force-pushed the 16057 branch from d557ec1 to a981a5d Compare June 24, 2017 06:25

Narretz suggested changes Jun 26, 2017

View reviewed changes

jbedard force-pushed the 16057 branch from a981a5d to 12a1ee7 Compare June 28, 2017 02:53

gkalpak reviewed Jun 29, 2017

View reviewed changes

jbedard force-pushed the 16057 branch from 12a1ee7 to 1ec77d0 Compare June 30, 2017 03:45

Narretz merged commit cdaa6a9 into angular:master Jul 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($timeout/$interval): do not trigger a digest when cancelling a $timeout/$interval #16064

fix($timeout/$interval): do not trigger a digest when cancelling a $timeout/$interval #16064

jbedard commented Jun 21, 2017

jbedard Jun 21, 2017

Narretz Jun 21, 2017

Narretz Jun 30, 2017

jbedard commented Jun 21, 2017

Narretz commented Jun 23, 2017

Narretz Jun 26, 2017 •

edited

Loading

Narretz Jun 26, 2017

Narretz Jun 27, 2017

jbedard commented Jun 28, 2017

gkalpak Jun 29, 2017

jbedard Jun 29, 2017

jbedard Jun 30, 2017

abobwhite commented Jan 26, 2018

jbedard commented Jan 27, 2018

fix($timeout/$interval): do not trigger a digest when cancelling a $timeout/$interval #16064

fix($timeout/$interval): do not trigger a digest when cancelling a $timeout/$interval #16064

Conversation

jbedard commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jun 21, 2017

Narretz commented Jun 23, 2017

Narretz Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abobwhite commented Jan 26, 2018

jbedard commented Jan 27, 2018

Narretz Jun 26, 2017 •

edited

Loading