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

feat(ngResource): support extending resources #7308

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yohaiai
Copy link

@yohaiai yohaiai commented Apr 30, 2014

A suggested solution for #4565

Request Type: feature

How to reproduce:

  • Array type action bug: create a resource with a custom action from type array, call that action on an instance of your custom resource. Exception is thrown.
  • Extending resource feature: You want the response of your action call to become a part of your resource instance. There is currently no straight forward way of doing this.

Component(s): ngResource

Impact: small

Complexity: small

This issue is related to:

When calling a Resource instance's GET actions such as $get and $query but mostly custom ones, it is sometimes needed to be able to extend the resource with the response data e.g:

entity.$children()
.then(function(){
    console.log(entity.children) //undefined
});
//the response did not become a part of the resource

Automatically setting the response on the resource with the property name is a bad idea since it can lead to unexpected overrides (the resource might have had a property named children) and generally it should be up to the user to decide whether to extend or not and how.

My approach involves leveraging the Resource's interceptor to achieve a both flexible and clean way to extend instances. By calling the interceptor's response function with the instance as it's context (this) it is possible to reference it and set the desired properties:

$resource('[...]/entity/:id', {}, {
    'children': {
        method: 'GET',
        url:'[...]/entity/:id/children',
        params:{id:'@id'},
        isArray:true,
        interceptor:{
            response: function(response){
                var data = response.data;
                this.children = data; //"this" references the entity resource
                return data;
            }
        }
    }
});

Update

As @dadleyy suggested,
I have added an optional param on actions called saveAs to give the user control over whether to save the response on the resource's instance and under what property name.

var Entity = $resource('[...]/entity/:id', {}, {
    'children': {
        method: 'GET',
        url:'[...]/children',
        params:{entityId:'@id'},
        saveAs:'children', //new optional property, value can be any string.
        isArray:true,
    }

var entity = Entity.get({id:123});
entity.$children(function(){
  //property name is the one defined in the *saveAs* param on the $children action.
  console.log(entity.children) //[child1, child2...]
});

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7308)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@caitp caitp added this to the Backlog milestone Apr 30, 2014
@dadleyy
Copy link

dadleyy commented May 1, 2014

I'm still not sold that saving them into the original resource would be a bad thing; wouldn't that mean the api has a model with a property that is also the name of another model? That situation doesn't seem too too likely. The automatic saving means you would be able to retain the use of your original object instead of using another -

$scope.user = ApiService.User.get({id:1});

$scope.getChildren = function() {
  // would populate the original's object's children property so 
  // your html bindings would update automatically
  $scope.user.$children();
}

instead of

$scope.user = ApiService.User.get({id:1});
$scope.getChildren = function() {
  $scope.user.$children() // returns the object or expects you to manipulate with then()
  .then(function(data) {
    this.children = data;
  });
}

I see what you're saying about it assuming a little too much though - it could definitely cause undesirable behavior for people who are expecting user.$children() to return a promise/new object and not tamper with the original. What if resources allowed a setting when defining custom actions do dictate the behavior:

ApiService.User = $resouce("/api/users/:id/:fn", {}, {
  children: {
    method: "GET",
    ...
    saveAs: "children"
    // or
    converge: true 
    // or
    noSave: true
  }
});

As devils advocate too, I think APIs who have these "tacked-on" methods for their models might not be considered by some to be best practice. Instead of having:

GET /api/users              [lists users]
GET /api/users/:id          [gets single user]
GET /api/users/:id/children [gets a single users' list of children]

I think some people might argue that the API should be designed without the user/:id/children function:

GET /api/users              [lists users]
GET /api/users/:id          [gets single user]

GET /api/children                   [lists children]
GET /api/children?user_id=<user_id> [gets a single users' list of children]
GET /api/children/:id               [gets single child]

That argument would mean that our suggestions were encouraging the use of bad practice api organization. I favor the first example but I know some people would be against tacking on those additional functions.

@yohaiai
Copy link
Author

yohaiai commented May 1, 2014

Hi @dadleyy,

thanks, I digg the saveAs suggestion, definitely cleaner. I do agree that some people won't consider the api/users/:id/children as good practices but the resource does not have to reflect the actual api. the actual endpoint can be /api/children or even /api2/children for that matter and can still be defined as an action on the api/users resource. Using the url option on the action it can really be anything.

Regardless, the bug that causes a crash when calling an array action on an instance of a resource must be resolved. If you allow users to defined such an action (isArray) it shouldn't crash.

@yohaiai yohaiai closed this May 1, 2014
@yohaiai yohaiai reopened this May 1, 2014
@yohaiai yohaiai added cla: yes and removed cla: no labels May 1, 2014
call ngResource's response interceptor with the resource's instance as the context
to give the user the abillity to reference it from within the interceptor function and set the response data on it

Closes angular#4565
@yohaiai yohaiai changed the title Extending resources feat(ngResource): support extending resources May 6, 2014
@dadleyy
Copy link

dadleyy commented May 7, 2014

Awesome stuff!

@yohaiai
Copy link
Author

yohaiai commented May 12, 2014

@mary-poppins so how to progress from here?

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

Successfully merging this pull request may close these issues.

6 participants