Promise returned from $timeout should not be chained because $$timeoutId is lost

[abobwhite]

I'm submitting a ...

• bug report
• feature request
• other

Current behavior:

$timeout returns a promise that has been "decorated" with a $$timeoutId that is then required when attempting to cancel the timeout via $timeout.cancel(promise). When chaining to this promise upon creation (i.e. then, catch, and/or finally), the promise returned from the chained function(s) is a new promise that does not contain $$timeoutId.

Expected / new behavior:

Ideally, Angular.js would not be adding properties willy-nilly to this promise. Secondly, it would be great if the chained calls would return back any extra properties set on the original promise (i.e. $$timeoutId). BUT, it would suffice if the documentation for $timeout could be enhanced to provide a note about this limitation.

Minimal reproduction of the problem with instructions:

Plnkr - Take note that this is built with a chained catch and causes the timeout to not be canceled. If you remove the chained catch or move the catch to a separate line after initial assignment, it cancels just fine.

AngularJS version:
1.6.6

Browser:
All

Anything else:
I strongly would consider this a defect but because I don't think you're going to change your underlying then, catch, and finally implementation to copy any ole' properties from the chained promise, documentation would suffice for any other future souls.

The guilty line of code is here: https://github.com/angular/angular.js/blob/master/src/ng/timeout.js#L66

[gkalpak]

Good catch, @abobwhite! I think this is a reasonable expectation.
(If we could only rely on WeakMaps 😉)

We could either use a HashMap (our internal implementation) to keep the promise<->timeoutId association (and ensure it gets cleaned up when the promise is resolved/rejected) or copy the $$timeoutId (and also $$intervalId) properties over to the new promise here:

angular.js/src/ng/q.js

Lines 320 to 331 in a8830d2

	then: function(onFulfilled, onRejected, progressBack) {
	if (isUndefined(onFulfilled) && isUndefined(onRejected) && isUndefined(progressBack)) {
	return this;
	}
	var result = new Promise();
	this.$$state.pending = this.$$state.pending || [];
	this.$$state.pending.push([result, onFulfilled, onRejected, progressBack]);
	if (this.$$state.status > 0) scheduleProcessQueue(this.$$state);
	return result;
	},

Since it will only affect promises returned by $timeout/$interval and people very rarely chain those, I don't think anyone will notice :)

[abobwhite]

Thanks @gkalpak Let me know if I can be of any service.

[gkalpak]

@abobwhite, if you fancy taking a stab at it, PRs are always welcome 😉

[petebacondarwin]

We are going to throw an error if you pass a promise to $timeout.cancel that doesn't contain a $$timeoutId - i.e. it was not the promise returned from the call to $timeout.

This requires a breaking change, so will go in 1.7.0.

[abobwhite]

@petebacondarwin Thanks for the heads up. The proposed solution seems like a good compromise given the EOL timeline for Angular.js and the (likely) pervasiveness of a change to passing along the $$timeoutId to chained promise methods.