fix($resource): remove (broken) support for promises as `timeout` #13462

gkalpak · 2015-12-07T22:49:23Z

This fixes #13393 by:

Reverting 7170f9d.
Update the docs to note that promise as timeout is not supported.
Log a warning when someone tries to use it.

The commits could/should be squashed before merging. We probably need a BC notice (since the broken support for promises as timeout has sneaked into v1.4.8, right ?

… promise This reverts commit 7170f9d. Fixes part of angular#13393.

petebacondarwin · 2015-12-07T22:59:34Z

src/ngResource/resource.js

- *     should abort the request when resolved.
+ *   - **`timeout`** – `{number}` – timeout in milliseconds.<br />
+ *     **Note:** In contrast to {@link ng.$http#usage $http.config}, {@link ng.$q promises} are
+ *     **not** supported in $resource, because the same value has to be re-used for multiple


Same value would be used?

I copied it from v1.5.x. I should change it there too, right ?

If it makes sense to you.

I updated it and changed it on master as well (3694390, da5db4b).

petebacondarwin · 2015-12-07T23:02:40Z

Some minor comments but lgtm

As discussed in #13462 (comment).

gkalpak · 2015-12-08T15:29:00Z

@petebacondarwin, I addressed the comments. What about a BC ?

Here's the situation:

Up to v1.4.7 (included), using a promise as timeout in $resource, would silently fail (i.e. have no effect).
In v1.4.8, using a promise as timeout will have the (buggy) behaviour described in Issue with canceling XHR request using promises - fixed #12525 #12657 (comment). (I.e. it will work as expected for the first time you resolve the promise and will cancel all subsequent requests after that - one has to re-create the resource class. This is not documented.)
With this change, using a promise as timeout will log a warning and ignore the timeout value.

Do we need a BC notice ?

It just occurred me: You said we should recommend an upgrade to 1.5 in the docs and warning message.

…rted Fixes part of angular#13393.

…ning Promises never worked correctly as values for `timeout` in `$resource`, because the same value has to be re-used for multiple requests (and it is not possible to `angular.copy()` a promise). Now (in addition to ignoring a non-numeric `timeout`), a warning is logged to the console using `$log.debug()`. Partly fixes angular#13393.

gkalpak · 2015-12-08T15:48:36Z

I updated the docs and the warning message to add a recommendation to upgrade to v1.5 if one needs support for cancellable $resource actions.

As discussed in #13462 (comment).

petebacondarwin · 2015-12-08T16:15:27Z

Yes, we should put a simple BC notice along the lines of your comment #13462 (comment)

I think in general it is safer to have BCs where they were not needed rather than the other way around

petebacondarwin · 2015-12-08T16:40:39Z

Landed as 4748652 and previous three commits.

revert: fix($resource): allow XHR request to be cancelled via timeout…

77d4a32

… promise This reverts commit 7170f9d. Fixes part of angular#13393.

googlebot added the cla: yes label Dec 7, 2015

petebacondarwin reviewed Dec 7, 2015
View reviewed changes

refactor($resource): change if-block to switch-block for readability

a758d58

gkalpak force-pushed the v1.4.x-fix-resource-promise-as-timeout branch from ed915fe to 66d1330 Compare December 8, 2015 13:24

gkalpak added a commit that referenced this pull request Dec 8, 2015

docs($resource): re-phrase warning message (has to --> would)

3694390

As discussed in #13462 (comment).

gkalpak force-pushed the v1.4.x-fix-resource-promise-as-timeout branch from 66d1330 to 7fe6201 Compare December 8, 2015 15:15

gkalpak added 2 commits December 8, 2015 17:43

docs($resource): add note about _promises as timeout_ not being suppo…

0c587ea

…rted Fixes part of angular#13393.

gkalpak force-pushed the v1.4.x-fix-resource-promise-as-timeout branch from 7fe6201 to 6d5141e Compare December 8, 2015 15:44

gkalpak added a commit that referenced this pull request Dec 8, 2015

docs($resource): re-phrase note in the docs (has to --> would)

da5db4b

As discussed in #13462 (comment).

petebacondarwin closed this Dec 8, 2015

gkalpak deleted the v1.4.x-fix-resource-promise-as-timeout branch December 8, 2015 19:01

gkalpak mentioned this pull request Dec 8, 2015

fix($resource): remove (broken) support for _promise as timeout_ #13393

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($resource): remove (broken) support for promises as `timeout` #13462

fix($resource): remove (broken) support for promises as `timeout` #13462

gkalpak commented Dec 7, 2015

petebacondarwin Dec 7, 2015

gkalpak Dec 7, 2015

petebacondarwin Dec 8, 2015

gkalpak Dec 8, 2015

petebacondarwin commented Dec 7, 2015

gkalpak commented Dec 8, 2015

gkalpak commented Dec 8, 2015

petebacondarwin commented Dec 8, 2015

petebacondarwin commented Dec 8, 2015

fix($resource): remove (broken) support for promises as timeout #13462

fix($resource): remove (broken) support for promises as timeout #13462

Conversation

gkalpak commented Dec 7, 2015

petebacondarwin Dec 7, 2015

Choose a reason for hiding this comment

gkalpak Dec 7, 2015

Choose a reason for hiding this comment

petebacondarwin Dec 8, 2015

Choose a reason for hiding this comment

gkalpak Dec 8, 2015

Choose a reason for hiding this comment

petebacondarwin commented Dec 7, 2015

gkalpak commented Dec 8, 2015

gkalpak commented Dec 8, 2015

petebacondarwin commented Dec 8, 2015

petebacondarwin commented Dec 8, 2015

fix($resource): remove (broken) support for promises as `timeout` #13462

fix($resource): remove (broken) support for promises as `timeout` #13462