Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding functionality to resources #54

Closed
kentcdodds opened this issue May 14, 2014 · 16 comments
Closed

Adding functionality to resources #54

kentcdodds opened this issue May 14, 2014 · 16 comments
Assignees
Milestone

Comments

@kentcdodds
Copy link
Contributor

I have a user resource and I want to add functionality to it to make things easier to work with. For example, with ngResource I did something like this:

var User = $resource(BaseUrl + '/api/v1/rest/users/:id', { id: '@_id' });
User.prototype.getDisplayName = function() {
  if (this.name.first && this.name.last) {
    return (this.name.first || '') + ' ' + (this.name.last || '');
  } else if (this.username) {
    return '@' + this.username;
  } else {
    return 'Anonymous';
  }
};

So, with the direction that angular-data is going, would you recommend:

  1. Have a UserControllerService or something that would act on the POJO
  2. Hook the lifecycle of the resource (like afterCreate) and return a new User(data) version of the POJO?

Looking forward to using this and hopefully providing good feedback and PRs 👍

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

I do have an open issue for computed properties #23, but haven't nailed down an easy mechanism for adding other custom behaviors. Technically, as long as the primary key of the pojo and its other attributes are still accessible with the dot operator (like normal), then you can wrap your pojo in whatever custom class you want.

The only issue is that you'll need to do it in more places than just in the afterCreate lifecycle hook, like every time you put data into the data store that doesn't go the afterCreate hook. For this reason, I might want to allow devs to (optionally) specify a class that angular-data should wrap around instances of a particular resource. This of course would incur a performance penalty.

I need to think about it some more. Other ideas?

@kentcdodds
Copy link
Contributor Author

Yeah, I was thinking that afterCreate wouldn't be the only place I'd need to do this. I'm going to try a UserControllerService or something and see how that goes, but I'm thinking that it would be nice to be able to operate with the resource itself...

My question would be how serious of a perf hit would it be? I'd imagine with a small number of objects, it wouldn't be noticeable. But with a ton, if it is a problem, then the user (dev) could handle it themselves (going with a controller service of some kind). I like the idea of providing a constructor as a config option. Seems like a good way to provide that functionality since people would probably be taking the perf hit when they attempt to wrap it themselves.

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

Another thought: perhaps it would work to simply accept a configuration object of methods that can be mixed in to the proto property of items in the data store. I'm not sure how "kosher" that is though...

Edit: this is probably a bad idea

Mutating the [[Prototype]] of an object is, by the nature of how modern JavaScript engines optimize property accesses, a very slow operation. If you care at all about performance, you should never mutate the [[Prototype]] of an object, either using this method or using Object.setPrototypeOf. Instead, create the object with the desired [[Prototype]] using Object.create. Furthermore, proto and the proto setter function are deprecated and should not be used.

@kentcdodds
Copy link
Contributor Author

Not sure where you got that quote, but good point. And you're right, it doesn't seem "kosher" either.... Hmmmmm...

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

The quote came from Mozilla's page on __proto__.

In that case perhaps the constructor function is the best option. But rather than accepting an actual function, I think it would be better to accept a hash of methods that would be mixed in the prototype of a constructor function provided by angular-data.

@kentcdodds
Copy link
Contributor Author

What if I wanted to do something special every time the model was created? You would have to have hooks like beforeInit and afterInit that would be part of that hash...

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

That's a good idea.

@kentcdodds
Copy link
Contributor Author

Do you think that's getting a little out of hand? Wouldn't it just be easier for people to define their own constructor, add anything they want to the prototype and then pass the constructor? Is there an issue with that approach that I'm missing?

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

I was thinking of the balance between freedom and protecting devs from that freedom. The issue with letting devs define their own constructor function means that they could potentially do anything with it, including defining all kinds of non-prototype properties on the constructed object that could collide and mix with the pojo attributes. Part angular-data's core design is that it doesn't decorate your data. I don't want to resort to getter/setter functions as a means of protecting the original pojo attributes from whatever the dev might be doing in that constructor function. It's a difference in philosophy. Any design intentions of mine become unenforceable with custom constructor functions.

While a constructor function would be the simplest to implement and give the dev the most control, it also opens things up for devs to shoot themselves in the foot.

Perhaps there is a solution that goes more down the UserControllerService route. Maybe something like this:

DS.defineResource({
  name: 'user',
  methods: {
    getDisplayName: function (user) {
      if (user.name.first && user.name.last) {
        return (user.name.first || '') + ' ' + (user.name.last || '');
      } else if (user.username) {
        return '@' + user.username;
      } else {
        return 'Anonymous';
      }
    }
  }
});
$scope.user = DS.get('user', 4);

$scope.displayName = DS('user').getDisplayName($scope.user)

where DS('user') just needs to be some way of accessing the resource definition that has the methods defined above.

Thoughts?

@kentcdodds
Copy link
Contributor Author

Point well taken.

For the UserControllerService thought, as a user of the api, I don't think that I would like that very much. It seems to me that it would be more reasonable to be able to do this:

$scope.user = DS.get('user', 4);
$scope.displayName = $scope.user.getDisplayName();

Here's what I think would be a good implementation:

  • If methods is provided, then a constructor function is created for the resource
  • The constructor first calls beforeInit, does whatever needs to be done (not sure what that would be) and then afterInit
  • Then any methods given in the methods hash would be added to that constructor's prototype.
  • Then any time data is gotten/updated, it is passed through the constructor before being passed back to a get call (for example)

What do you think?

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

I like it.

If methods is not provided, then the POJOs are not passed through any constructor, but left alone? This would keep things fast for devs who don't want to add their own behavior.

@kentcdodds
Copy link
Contributor Author

I like that as well. I'm just wondering if there's a way to make a more clear distinction between POJOs and constructed objects... Do you think it would be confusing for devs coming in thinking "Now is this user object an instanceof User or is it just instanceof Object?" You know what I mean? Would that be a stumbling block?

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

I think the rule will be:

methods provided? Object in DS? instanceOf Model?
yes yes yes
yes no no
no yes no
no no no

Will that easy enough to understand?

@kentcdodds
Copy link
Contributor Author

Yeah that makes sense, and if it's not in the DS then wouldn't it be null/undefined (I don't know how the api works currently).

@jmdobry
Copy link
Member

jmdobry commented May 14, 2014

DS.get(resourceName, id) returns undefined if the item isn't in the store.

I can start on implementing this tonight. I'm now in the process of testing DS.bindOne and DS.bindAll.

@kentcdodds
Copy link
Contributor Author

👍 sounds awesome. Thanks!

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

No branches or pull requests

2 participants