From 37dc1d3f74f81e9b0a4a7d9010bab8b266b49da3 Mon Sep 17 00:00:00 2001 From: Alexander Shtuchkin Date: Wed, 17 Apr 2013 02:08:04 +0400 Subject: [PATCH] feat(resource): rework promise-based api Change promise API to be more consistent with other parts of Angular and with abstraction layers. Major changes: * Exposed instance.$promise as the most recent server interaction promise. * $promise is resolved with resource instance, not http response object. This is more consistent to success/error callbacks and route when/resolve clause. * Changed instance.$resolved to be true all the time after first server interaction finished. * Removed instance.$then and response.resource as they are not needed anymore. * Instance actions now return $promise to allow easy chaining. Tests & Docs are updated accordingly. --- src/ngResource/resource.js | 97 +++++++++++---------------- test/ngResource/resourceSpec.js | 115 ++++++++++++++++++++------------ 2 files changed, 113 insertions(+), 99 deletions(-) diff --git a/src/ngResource/resource.js b/src/ngResource/resource.js index 05818271d007..8b367abb34d6 100644 --- a/src/ngResource/resource.js +++ b/src/ngResource/resource.js @@ -122,24 +122,26 @@ * - non-GET "class" actions: `Resource.action([parameters], postData, [success], [error])` * - non-GET instance actions: `instance.$action([parameters], [success], [error])` * + * Success callback is called with (value, responseHeaders) arguments. Error callback is called + * with (httpResponse) argument. + * + * Class actions return empty instance (with additional properties below). + * Instance actions return instance.$promise. * * The Resource instances and collection have these additional properties: * - * - `$then`: the `then` method of a {@link ng.$q promise} derived from the underlying - * {@link ng.$http $http} call. + * - `$promise`: the {@link ng.$q promise} of most recent server interaction results, after + * Resource.action or instance.$action were called. * - * The success callback for the `$then` method will be resolved if the underlying `$http` requests - * succeeds. + * On success, the promise is resolved with the same resource instance or collection object, + * updated with data from server. This makes it easy to use in + * {@link ng.$routeProvider resolve section of $routeProvider.when()} to defer view rendering + * until the resource(s) are loaded. + * + * On failure, underlying {@link ng.$http http response} object is given as the reason. * - * The success callback is called with a single object which is the {@link ng.$http http response} - * object extended with a new property `resource`. This `resource` property is a reference to the - * result of the resource action — resource object or array of resources. - * - * The error callback is called with the {@link ng.$http http response} object when an http - * error occurs. - * - * - `$resolved`: true if the promise has been resolved (either with success or rejection); - * Knowing if the Resource has been resolved is useful in data-binding. + * - `$resolved`: true after first server interaction is completed (either with success or rejection), + * undefined before that. Knowing if the Resource has been resolved is useful in data-binding. * * @example * @@ -260,7 +262,7 @@ */ angular.module('ngResource', ['ng']). - factory('$resource', ['$http', '$parse', function($http, $parse) { + factory('$resource', ['$http', '$parse', '$q', function($http, $parse, $q) { var DEFAULT_ACTIONS = { 'get': {method:'GET'}, 'save': {method:'POST'}, @@ -389,14 +391,10 @@ angular.module('ngResource', ['ng']). } forEach(actions, function(action, name) { - action.method = angular.uppercase(action.method); - var hasBody = action.method == 'POST' || action.method == 'PUT' || action.method == 'PATCH'; + var hasBody = /^(POST|PUT|PATCH)$/i.test(action.method); + Resource[name] = function(a1, a2, a3, a4) { - var params = {}; - var data; - var success = noop; - var error = null; - var promise; + var params = {}, data, success, error; switch(arguments.length) { case 4: @@ -428,11 +426,11 @@ angular.module('ngResource', ['ng']). break; case 0: break; default: - throw "Expected between 0-4 arguments [params, data, success, error], got " + + throw "Expected up to 4 arguments [params, data, success, error], got " + arguments.length + " arguments."; } - var value = this instanceof Resource ? this : (action.isArray ? [] : new Resource(data)); + var value = data instanceof Resource ? data : (action.isArray ? [] : new Resource(data)); var httpConfig = {}, promise; @@ -444,15 +442,8 @@ angular.module('ngResource', ['ng']). httpConfig.data = data; route.setUrlParams(httpConfig, extend({}, extractParams(data, action.params || {}), params), action.url); - function markResolved() { value.$resolved = true; } - - promise = $http(httpConfig); - value.$resolved = false; - - promise.then(markResolved, markResolved); - value.$then = promise.then(function(response) { + value.$promise = promise = $http(httpConfig).then(function(response) { var data = response.data; - var then = value.$then, resolved = value.$resolved; if (data) { if (action.isArray) { @@ -462,44 +453,34 @@ angular.module('ngResource', ['ng']). }); } else { copy(data, value); - value.$then = then; - value.$resolved = resolved; } } + value.$resolved = true; + value.$promise = promise; + (success||noop)(value, response.headers); - response.resource = value; - return response; - }, error).then; + return value; + + }, function(response) { + value.$resolved = true; + value.$promise = promise; + + (error||noop)(response); + + return $q.reject(response); + }); return value; }; - Resource.prototype['$' + name] = function(a1, a2, a3) { - var params = extractParams(this), - success = noop, - error; - - switch(arguments.length) { - case 3: params = a1; success = a2; error = a3; break; - case 2: - case 1: - if (isFunction(a1)) { - success = a1; - error = a2; - } else { - params = a1; - success = a2 || noop; - } - case 0: break; - default: - throw "Expected between 1-3 arguments [params, success, error], got " + - arguments.length + " arguments."; + Resource.prototype['$' + name] = function(params, success, error) { + if (isFunction(params)) { + error = success; success = params; params = {}; } - var data = hasBody ? this : undefined; - Resource[name].call(this, params, data, success, error); + return Resource[name](params, this, success, error).$promise; }; }); diff --git a/test/ngResource/resourceSpec.js b/test/ngResource/resourceSpec.js index 225f96a165ba..5507449c3099 100644 --- a/test/ngResource/resourceSpec.js +++ b/test/ngResource/resourceSpec.js @@ -457,58 +457,56 @@ describe("resource", function() { describe('single resource', function() { - it('should add promise $then method to the result object', function() { + it('should add $promise to the result object', function() { $httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'}); var cc = CreditCard.get({id: 123}); - cc.$then(callback); + cc.$promise.then(callback); expect(callback).not.toHaveBeenCalled(); $httpBackend.flush(); - var response = callback.mostRecentCall.args[0]; + var value = callback.mostRecentCall.args[0]; - expect(response.data).toEqual({id: 123, number: '9876'}); - expect(response.status).toEqual(200); - expect(response.resource).toEqualData({id: 123, number: '9876', $resolved: true}); - expect(typeof response.resource.$save).toBe('function'); + expect(value).toEqualData({id: 123, number: '9876'}); + expect(typeof value.$save).toBe('function'); }); - it('should keep $then around after promise resolution', function() { + it('should keep $promise around after resolution', function() { $httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'}); var cc = CreditCard.get({id: 123}); - cc.$then(callback); + cc.$promise.then(callback); $httpBackend.flush(); - var response = callback.mostRecentCall.args[0]; + var value = callback.mostRecentCall.args[0]; callback.reset(); - cc.$then(callback); + cc.$promise.then(callback); $rootScope.$apply(); //flush async queue - expect(callback).toHaveBeenCalledOnceWith(response); + expect(callback).toHaveBeenCalledOnceWith(value); }); - it('should allow promise chaining via $then method', function() { + it('should allow promise chaining', function() { $httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'}); var cc = CreditCard.get({id: 123}); - cc.$then(function(response) { return 'new value'; }).then(callback); + cc.$promise.then(function(value) { return 'new value'; }).then(callback); $httpBackend.flush(); expect(callback).toHaveBeenCalledOnceWith('new value'); }); - it('should allow error callback registration via $then method', function() { + it('should allow $promise error callback registration', function() { $httpBackend.expect('GET', '/CreditCard/123').respond(404, 'resource not found'); var cc = CreditCard.get({id: 123}); - cc.$then(null, callback); + cc.$promise.then(null, callback); $httpBackend.flush(); var response = callback.mostRecentCall.args[0]; @@ -522,10 +520,10 @@ describe("resource", function() { $httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'}); var cc = CreditCard.get({id: 123}); - expect(cc.$resolved).toBe(false); + expect(cc.$resolved).toBe(undefined); - cc.$then(callback); - expect(cc.$resolved).toBe(false); + cc.$promise.then(callback); + expect(cc.$resolved).toBe(undefined); $httpBackend.flush(); @@ -537,69 +535,104 @@ describe("resource", function() { $httpBackend.expect('GET', '/CreditCard/123').respond(404, 'resource not found'); var cc = CreditCard.get({id: 123}); - cc.$then(null, callback); + cc.$promise.then(null, callback); $httpBackend.flush(); expect(callback).toHaveBeenCalledOnce(); expect(cc.$resolved).toBe(true); }); + + + it('should keep $resolved true in all subsequent interactions', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'}); + var cc = CreditCard.get({id: 123}); + $httpBackend.flush(); + expect(cc.$resolved).toBe(true); + + $httpBackend.expect('POST', '/CreditCard/123').respond(); + cc.$save({id: 123}); + expect(cc.$resolved).toBe(true); + $httpBackend.flush(); + expect(cc.$resolved).toBe(true); + }); + + + it('should return promise from action method calls', function() { + $httpBackend.expect('GET', '/CreditCard/123').respond({id: 123, number: '9876'}); + var cc = new CreditCard({name: 'Mojo'}); + + expect(cc).toEqualData({name: 'Mojo'}); + + cc.$get({id:123}).then(callback); + + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnceWith(cc); + expect(cc).toEqualData({id: 123, number: '9876'}); + callback.reset(); + + $httpBackend.expect('POST', '/CreditCard').respond({id: 1, number: '9'}); + + cc.$save().then(callback); + + $httpBackend.flush(); + expect(callback).toHaveBeenCalledOnceWith(cc); + expect(cc).toEqualData({id: 1, number: '9'}); + }); }); describe('resource collection', function() { - it('should add promise $then method to the result object', function() { + it('should add $promise to the result object', function() { $httpBackend.expect('GET', '/CreditCard?key=value').respond([{id: 1}, {id: 2}]); var ccs = CreditCard.query({key: 'value'}); - ccs.$then(callback); + ccs.$promise.then(callback); expect(callback).not.toHaveBeenCalled(); $httpBackend.flush(); - var response = callback.mostRecentCall.args[0]; + var value = callback.mostRecentCall.args[0]; - expect(response.data).toEqual([{id: 1}, {id :2}]); - expect(response.status).toEqual(200); - expect(response.resource).toEqualData([ { id : 1 }, { id : 2 } ]); - expect(typeof response.resource[0].$save).toBe('function'); - expect(typeof response.resource[1].$save).toBe('function'); + expect(value).toEqualData([{ id : 1 }, { id : 2 }]); + expect(typeof value[0].$save).toBe('function'); + expect(typeof value[1].$save).toBe('function'); }); - it('should keep $then around after promise resolution', function() { + it('should keep $promise around after resolution', function() { $httpBackend.expect('GET', '/CreditCard?key=value').respond([{id: 1}, {id: 2}]); var ccs = CreditCard.query({key: 'value'}); - ccs.$then(callback); + ccs.$promise.then(callback); $httpBackend.flush(); - var response = callback.mostRecentCall.args[0]; + var value = callback.mostRecentCall.args[0]; callback.reset(); - ccs.$then(callback); + ccs.$promise.then(callback); $rootScope.$apply(); //flush async queue - expect(callback).toHaveBeenCalledOnceWith(response); + expect(callback).toHaveBeenCalledOnceWith(value); }); - it('should allow promise chaining via $then method', function() { + it('should allow promise chaining', function() { $httpBackend.expect('GET', '/CreditCard?key=value').respond([{id: 1}, {id: 2}]); var ccs = CreditCard.query({key: 'value'}); - ccs.$then(function(response) { return 'new value'; }).then(callback); + ccs.$promise.then(function(value) { return 'new value'; }).then(callback); $httpBackend.flush(); expect(callback).toHaveBeenCalledOnceWith('new value'); }); - it('should allow error callback registration via $then method', function() { + it('should allow $promise error callback registration', function() { $httpBackend.expect('GET', '/CreditCard?key=value').respond(404, 'resource not found'); var ccs = CreditCard.query({key: 'value'}); - ccs.$then(null, callback); + ccs.$promise.then(null, callback); $httpBackend.flush(); var response = callback.mostRecentCall.args[0]; @@ -613,10 +646,10 @@ describe("resource", function() { $httpBackend.expect('GET', '/CreditCard?key=value').respond([{id: 1}, {id: 2}]); var ccs = CreditCard.query({key: 'value'}, callback); - expect(ccs.$resolved).toBe(false); + expect(ccs.$resolved).toBe(undefined); - ccs.$then(callback); - expect(ccs.$resolved).toBe(false); + ccs.$promise.then(callback); + expect(ccs.$resolved).toBe(undefined); $httpBackend.flush(); @@ -628,7 +661,7 @@ describe("resource", function() { $httpBackend.expect('GET', '/CreditCard?key=value').respond(404, 'resource not found'); var ccs = CreditCard.query({key: 'value'}); - ccs.$then(null, callback); + ccs.$promise.then(null, callback); $httpBackend.flush(); expect(callback).toHaveBeenCalledOnce(); expect(ccs.$resolved).toBe(true);