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

$resource with promises #2607

Merged
merged 1 commit into from
May 23, 2013
Merged

Conversation

vojtajina
Copy link
Contributor

This is just finishing #2060.

My changes, that will get squashed into the first commit:

  • change .$resolved to be boolean (true/false), rather than undefined/true
  • keep the original promise (do not update .$promise after subsequent instance calls)
  • success callbacks gets response (with value as .resource), instead of direct value (this is just reverting to the original behavior of $resource)
  • docs: reflect these changes
  • some style/formatting (reverting trailing white spaces, etc.)

Passsing whole response into the promise:
I did this, because I think it is very helpful to have access to the http response. More than that, people might be already relying on it.
The reason for passing just the value (resource instance) was to simplify $route resolve, but I think it's not that worse (unless I'm missing something):

// passing whole response
MyController.resolve = function(MyResource) {
  return MyResource.get(...).promise.then(function(response) {
    return response.resource;
  });
};

// passing direct resource instace
MyController.resolve = function(MyResource) {
  return MyResource.get(...).promise;
};

Before merging:

  • squash
  • add BREAKING CHANGE message

@IgorMinar
Copy link
Contributor

resolving the promise with http response is not ideal because in 99% of the cases you don't care about the http response. it also breaks the abstraction layers since in the higher level abstraction you primarily deal with low level object.

your examples show how much more complicated the resolve code becomes when you resolve the promise with low-level http response.

@IgorMinar
Copy link
Contributor

someone proposed that we expose the http response on the resource instance but that's not ideal because the resource instance property would have to be overriden every time the resource is updated. so there would be no guarantee that you are reading the response you want to be reading.

a better thing might be to provide success/error promise sugar methods just like we do with $http and expose headers there.

another alternative is to expose the headers/response object on this of the promise.

Users.query().$promise.then(function(users) {
  this.response.headers ....
});

would that work?

@vojtajina
Copy link
Contributor Author

transformResponse has only access to the response.data and headers (eg. no status code, neither any info about the request such as requested url)

If the headers are enough, then it's doable, however... it's not the best experience ;-)

// hack to parse JSON ;-)
app.config(function($httpProvider, $provide) {
  $provide.value('parseJSON', $httpProvider.defaults.transformResponse[0]);
});

app.factory('Project', function($resource, parseJSON) {
  var Project = $resource('https://api.mongolab.com/api/1/databases/angularjs/collections/projects/:id', {
    apiKey: '4f847ad3e4b08a2eed5f3b54',
  }, {
    query: {
      method: 'GET', isArray: true,
      transformResponse: [parseJSON, function(data, header) {
        // this is before $resource instantiates Resource instance,
        // so it's tricky to communicate the value into my app ;-)
        data.forEach(function(item) {
          item.someHeader = header('pragma');
        });

        return data;
      }]
    }
  });

  // ...

  return Project;
});

@vojtajina
Copy link
Contributor Author

So my question to everybody: What are the use cases for having access to headers / status code / http config object when using $resource.

@honzajde
Copy link

This maybe slightly off-topic: when working with non-trivial use-cases you might end-up adding additional properties on the downloaded JSON data (e.g. child._parent = parent). Then before save I want to strip them off, ideally when marshaling JSON like this: JSON.stringify(data, censor), so that the model does not get modified. This leads in my code in many places to actually using $http, cause there is no? way to hook into marshalling process with $resource.

@IgorMinar
Copy link
Contributor

use case:

You have User resource. You create a new instance via new User and then you call $save(). This should issue a POST request to /users/. The server responds with HTTP 201 Created, empty body and header Location: /users/123. The User $resource should then take this response, parse out 123 from the location header and set the id property of the user object.

here is an idea: what if we added per-request http interceptors and used those to bridge the http and resource? this would work both for the header use case and @honzajde's use case.

@vojtajina
Copy link
Contributor Author

@honzajde $resource ignores all properties starting with $, so if you call it child.$parent it won't be sent to the server. Or you can use transformRequest.

Using reponseInterceptor per request is imho the same as doing $http(...).then(interceptor), no ?

We could have responseInterceptor for $resource, which would basically do $http(...).then(theResourceStuff).then(resourceInterceptor), with default interceptor being something like:

var defaultResourceInterceptor = function(response) {
  return response.resource;
};

Then, user can override this default interceptor, which gives him access to everything. The default interceptor makes it easy to work with $route resolve. That SGTM and I'm gonna implement this.

I don't think it solves @honzajde's use case, as his problem is more about client->server serialization (which can be done using transformResponse).

@honzajde
Copy link

@vojtajina oh, I have not realized, that $resoruce is using $http underneah. Global transformRequest does the job. Thanks.

As far as I know I can not set transformRequest for a particular use of $resource, right?
As with $resource being just a syntactic sugar for $http it would be maybe convenient to be able to get to $http's config object per $resrouce definition or per $resource's method call . Just an idea...

The around interceptor for $http has been implmented if I am not wrong... So in the global around interceptor I can do even "url rewriting", that does not make sense per request.

@IgorMinar
Copy link
Contributor

@vojtajina I like that quite a bit. let's try it, but make sure that it can be defined on both "per-resource" as well as "per-resource-action" levels (the second overriding the first one)

@vojtajina
Copy link
Contributor Author

I agree, that having both transformResponse/Request and interceptorResponse/Request is confusing. I don't like it either, but that's just something we did over time ;-) We can simplify that in a separate PR, by removing transforms.

Adding per http request interceptor is fine with me, but note, that it's kind of redundant:

$http.get('/url', {
  interceptor: {
    response: function(response) {
      // transform response
}}});

// this is the same
$http.get('/url').then(function(response) {
  // transform response
});

Ad. @IgorMinar 's comment on "interceptor when defined overides the default interceptor". I think that's an implementation detail, why would user care about it ? If so, he can go and see the source code. Just my $0.02, I'm fine adding it into the docs.

Ad. unifying the syntax of $http and $resource interceptors. In general, I agree that unified syntax is better. $http interceptors are injectable, because they are registered in config phase, so there is no other way. Resource is constructed in runtime phase. Let's say you wanna have an interceptor, shared between multiple resources. I would do something like this:

module.factory('authInterceptor', function($http, $q) {
  return function(response) {
    // whatever
  };
});

module.factory('UserResource', function($resource, authInterceptor) {
  return $resource('/user/:id', {}, {
    query: {
      method: 'GET', 
      interceptor: {response: authInterceptor}
    }
  };
});

module.factory('OtherResource', function($resource, authInterceptor) {
  return $resource('/other/:id', {}, {
    query: {
      method: 'GET', 
      interceptor: {response: authInterceptor}
    }
  };
});

I'm adding the unit test for the "id in headers" use case.

@vojtajina
Copy link
Contributor Author

Conspiracy Theory:
The interception happens after the resource does its stuff (eg. updating the instance with values from the server). Because promises are async, the interception happens in following async queue. Is it gonna confuse ng-repeat ? (eg. if I set the id of the object from headers, inside interceptor, is ng-repeat gonna re-render)...

@IgorMinar
Copy link
Contributor

@vojtajina that's not an issue since both the resource stuff (merge) and interceptor are executed from the same async queue run before watchers fire

- Instance or collection have `$promise` property which is the initial promise.
- Add per-action `interceptor`, which has access to entire $http response object.

BREAKING CHANGE: resource instance does not have `$then` function anymore.

Before:

Resource.query().$then(callback);

After:

Resource.query().$promise.then(callback);

BREAKING CHANGE: instance methods return the promise rather than the instance itself.

Before:

resource.$save().chaining = true;

After:

resource.$save();
resourve.chaining = true;

BREAKING CHANGE: On success, promise is resolved with the resource instance rather than http
response object.

Use interceptor to access the http response object.

Before:

Resource.query().$then(function(response) {...});

After:

var Resource = $resource('/url', {}, {
  get: {
    method: 'get',
    interceptor: {
      response: function(response) {
        // expose response
        return response;
      }
    }
  }
});
@vojtajina vojtajina merged commit 05772e1 into angular:master May 23, 2013
@vojtajina vojtajina deleted the pr-resource-promise branch May 23, 2013 21:32
@IgorMinar
Copy link
Contributor

@bclinkinbeard angular v1.x doesn't follow semantic versioning for historical reasons. versions 1.1.x are unstable as properly communicated in changelog and other places. the reasons for these breaking changes is to improve the promise apis that were introduced recently in another unstable release. once 1.1.x becomes 1.2, these apis will be considered safe to use.

@eddiemonge
Copy link
Contributor

so how do we actually get the response data now? Before I was able to do (although it might be incorrect, I dont know):

Resource.save({resourceInfo}, {data}, function(response) { response.status. response.otherdata}, function(response) { response.error});

With the first anon function being the success callback and the second being an error callback.

@atul221282
Copy link

i am still looking for an answer and some nice explanation on how to return promise in resource with some nice error handling.

@kevinrenskers
Copy link

Same question here. How do I get the status code in the success callback?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants