feat($resource): expose $promise and $response in promise-based api

[ashtuchkin]

This is a WIP to show a possible $resource promise-based api discussed in dba6bc7

Main changes:

• $promise and $response (http response) are exposed on resource instance object in addition to $then and $resolved.
• on success, the promise resolves to the same resource instance object filled with data. This is as expected to enable using it in resolve section of $route.when().

Arguably, $then and $response.resource are not needed anymore, what do you think?

[IgorMinar]

• don't expose $response
• resolve with response object not value
• add test for $promise

PR Checklist (Minor Feature)

• Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
• Feature improves existing core functionality
• API is compatible with existing Angular apis and relevant standards (if applicable)
• PR doesn't contain a breaking change
• PR contains unit tests
• PR contains e2e tests (if suitable)
• PR contains documentation update
• PR passes all tests on Travis (sanity)
• PR passes all tests on ci.angularjs.org (cross-browser compatibility)
• PR is rebased against recent master
• PR is squashed into one commit per logical change
• PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
• All changes requested in review have been implemented

[ashtuchkin]

Thanks for the review, Igor!

Lets discuss returning value vs http response object from the promise. I chose to return value in this PR because:

1. It is consistent with success/error callbacks. Success is called with value, error is called with response.
2. It is more consistent with the $route.when(url, {resolve: ...}). That way, when you want the controller and view rendering to be deferred until resource is loaded, you just give it the $promise and it returns value, as expected. Like this:

  .when("products", {
    templateUrl: "products.html",
    resolve: {
      products: function(Product) {
        return Product.query().$promise;
      }
    },
    controller: function($scope, products) {
      $scope.products = products;
    } 
  })

If it were to return http response, we will have to use something like$scope.products = productsHttpResponse.resource in controller.

In general, to answer that question, we should ask ourselves: a promise of what do we want to provide in resource.$promise? I view it as a promise of initial data because it's the abstraction layer that $resource module provides. It can also be viewed as a promise of underlying http response, but I think in this case it should be named $httpResponsePromise to avoid confusion (of abstraction layers). Or $httpPromise at least.

What do you think?

What also bothers me right now is that the $promise is tied to initial request. What should happen when the user calls $get() on the resource to update it? Maybe we should replace it with new promise (it could easily get complicated if several requests will be in flight at the same time)? Or leave it as is and just don't bother?

The tests and docs will be updated as soon as we get a decision about this.

[jonbcard]

@ashtuchkin I'm +1 with you in wishing that the promise on $resource would be a promise for the resource and not something to expose the $http guts (#1547 (comment)). If someone needs the $http level stuff I'd wonder why they are bothering to use ngResource abstraction in the first place. It just isn't intuitive behaviour (or maybe just not intuitive naming?) to me .

I've also always found there to be some weirdness with this whole thing wrt. what happens what you call instance methods like $save().

If totally breaking the API was an option my vote would be to:

• Have $promise, $resolved and $then exposed on resource object. Let these be a promise for the resource and not leak out an $http promise. These properties would be tied to the initial request only.
• (And here is where I'm imagining the Angular team cringing) Change resource so that instance methods like $get or $save return a new promise instead of the resource object. Yeah -- I recognize this is a pretty big breaking change, which is why I don't think it would happen, but to me it seems like the best way to reconcile $resource with the promise model.

[IgorMinar]

@ashtuchkin you make some good arguments, but here are the constraints that I'm working with:

1. the http headers sometimes contain important information that must be available to success callbacks. for example when creating a new resource the server can respond with http 204 and send header Location with the URI of the newly created resource.
2. the promise can be resolved only to a single value so that chaining and other stuff works well. So we can't resolve the promise with two arguments: the resource object and response object.

@jonbcard as described above, there must be a way to get hold of the http info.

[jonbcard]

@IgorMinar yeah -- I get where you're coming from on this, but it my own head it makes more sense just to expose important information (e.g. headers) as a property of a resource response rather than leaking the $http abstraction.

Edit: I guess what I just described isn't much different from what you're looking for. It just a question of whether a resource object or the http response is the top level in the object hierarchy provided to the callback.

[IgorMinar]

@jonbcard fair enough. I'm starting to get convinced.

[jonbcard]

@IgorMinar That's good. Maybe @ashtuchkin is already close on this, then, if we can fully convince you. :).

As @ashtuchkin brought up, I also think there is still an open question about how to define what is supposed to happen to an existing Resource's promise properties when you call an instance method on it. I didn't look carefully at this PR but my assumption is that they will get overwritten. Whatever behaviour happens here, can we make it well defined as part of the doc and spec? Its important especially if you want to do something like this:

<div ng-show="product.$resolved">
    <form>
        <input ng=model="product.name">
        ...
        <button ng-click="product.$save()">
    </form>
</div>

If we're overwriting the resource $resolved/$promise properties, the form would quickly disappear while the save action happens then reappear when completed. If people at least know that this is expected, they can at least code around it / avoid using $resolved in this way .

[IgorMinar]

@jonbcard we should not overwrite the props as that would defeat the main usecase for $resolve which is what you captured in the example above.

breaking the action method api is very tempting now.. :)

[jonbcard]

@IgorMinar You talking about changing the instance method API (e.g. $save()) to not do weird things with $resolved after the initial response, right? If so, you have my one small but enthusiastic vote for doing that. Would you change it to return a promise, or a new/different resource (instead of the same instance, as it does now)? I guess the second option may be lower impact. Note: If I just missed something and the PR already handles this acceptably someone please set me straight!

@IgorMinar @ashtuchkin Please let me know if there is anything I can do to help out on this. (e.g. if a new PR would help you contemplate the change).

[ashtuchkin]

Guys I've updated the PR, please see. Main changes:

• $resolved is not changed anymore after set to true (+added a test).
• explicitly state that $promise and $response are for the most recent server interaction, not only the first one.
• instance.$action methods return promise, allowing to write something like cc.$save().then(...) or to interact with scopes without empty object $scope.data = cc.$get();
• removed $then and response.resource. They are not needed IMHO.
• slightly refactored surrounding code.

I've also updated the documentation, please check below. Maybe we need to add examples, what do you think?

The action methods on the class object or instance object can be invoked with the following
parameters:
 - HTTP GET "class" actions: `Resource.action([parameters], [success], [error])`
 - 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:
 - `$promise`: the {@link ng.$q promise} of most recent server interaction results, after
   Resource.action or instance.$action were called.

   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.

 - `$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.

 - `$response`: the {@link ng.$http http response} object from most recent server interaction.
   Use it to get access to headers and other raw http data.

[ashtuchkin]

Rebased to master.

[IgorMinar]

I discussed this with the rest of the team today and I'd like to explore one more path: what if the promise api always resolved to resource instances only and no http anything would be exposed anywhere in the resource api, except for the place where you define your resource.

In the resource definition you'd register an extra http transform that would get access to the http primitives and it would be job of this transform to construct a new resource instance or update an existing one.

that way the http stuff is encapsulated in the resource definition and doesn't leak into the code that uses the resource instances (controllers or services).

[ashtuchkin]

I totally agree. That way we can simplify the code even more. Would you like me to do that? Do you want to keep the other logic as it is done now?

[IgorMinar]

Yes please. Can you give it a shot?

Make the initial changes and then commit it as a new commit into this branch, so that I can see how things changed from the current version.

[tpodom]

Would it be possible to make the transformers support both transformer prior to sending data and to transform the data coming back? I wrote a rails resource factory to allow us to support root wrapping and camel/underscore based naming translation and the key part of that was adding transformers/interceptors to perform that logic.

The other motivation was to expose promises but I like what you guys are doing here with promises better where the query/get would return an object with the promise available on it and the instance methods would return the promise. Returning the object from the query/get is something we are going to be changing in my project because it's definitely more convenient for those to be a reference to what gets populated on success.

[jgrund]

+1 to transforming data before send / receive.

When you construct / update a resource instance it would also be really useful to define methods that could act on that resource data.

[trygvis]

+1, this latest version is exactly what I'm looking for.

[stevedomin]

@ashtuchkin are you still working on that ?

[ashtuchkin]

Sorry for the delay, guys.
@IgorMinar I've pushed the version where the http response ($resource) is removed, but I'm not sure what to do with:

1. Success callback - it is given the http headers
2. Error callback - it is given the http response object now.
3. Rejected $promise - it is also given the http response. And I don't know what would be a better alternative that is meaningful for error handling.
4. transformRequest/Response are working without changes in this PR, but they are limited as f.ex. transformResponse is not given an http status code and it cannot change error/success status of the request. I believe interceptors could be a much better idea, but they are global unfortunately.

Let me know how you want to proceed with this PR.

@jgrund I agree that instance methods would be a great addition, making ngResource closer to a full-blown Model component, but I believe it's a scope for another PR.

[jgrund]

@ashtuchkin I agree, hope to see these changes move forward so methods can be added after.

[klebba]

If I understand this change correctly; resource.$save().$then() will become resource.$save().$promise.then() -- is that right? One way or another $then() is bizarre; it should be renamed to then()

Keep up the great work!

[ashtuchkin]

@klebba Thank you! Correction: it will be resource.$save().then(), because $ methods on resource instances return promise.

[klebba]

That makes sense, great!

[IgorMinar]

fyi we've rebased this PR and are experimenting with some more changes via PR #2607

[vojtajina]

Merged in #2607 - thanks @ashtuchkin !