Make overriding fetching attributes and relationships easier #653

Closed
wants to merge 2 commits into
from

Projects

None yet

6 participants

Contributor
drogus commented Jan 22, 2013

Currently there is no easy way to hook into the place where get('attribute') is called (where attribute can be also relationship). This is needed in order to extend ember-data with custom behavior in those points.

My use case is "incomplete" record. On https://travis-ci.org we use a notion on "incomplete" records - record that has an id and a data populated in some way (in our case it's pusher), but this data may not contain all of the attributes. When attribute that can be missing is fetched, full record is fetched through the API. That way even if we use attributes or relationships that are not set by pusher, we don't risk that data is outdated.

I would like to send a pull request for incomplete functionality as well, but figuring out how should it work to be useful also for others and pushing it into the framework will probably take some time. The changes in this pull request are pretty straightforward and will enable me to implement incomplete records much easier (ie. no monkey patching).

Owner
igorT commented Apr 15, 2013

This seems good to me. @tomdale any objections? Could you rebase to curent master so I can merge?

Owner
tomdale commented Apr 15, 2013

I don't feel comfortable making this the public API. We should discuss an implementation for "incomplete" records (what we've historically referred to as "extended attributes.")

drogus added some commits Jan 22, 2013
@drogus drogus Allow to override getting attribute on a DS.Model
Currently there is no easy way to override getAttr function, which is
used in the computed property returned by DS.attr and therefore it's
hard hook into the moment of getting attribute.

This commit moves getAttr into the model to allow access to this
function. For example:

    var Person = DS.Model.extend({
      name: DS.attr('string'),

      getAttr: function(key, options) {
        console.log("Trying to feth attribute '" + key + "'.);
      }
    });

    // assuming that person is of Person type
    person.get('name')

Such code will output following text to the console:

    Trying to fetch attribute 'name'.
937f292
@drogus drogus Allow to override fetching relationships on a DS.Model
In order to make modifications in how relationships work, this commit
introduces 2 methods that are called when hasMany or belongsTo
relationship attributes are fetched from the object, respectively:
getHasMany and getBelongsTo.

Those methods are called on DS.Model instance when computed property for
a relationship is called.
0c0a71a
Contributor
drogus commented May 22, 2013

Sorry for delayed answer, I have a hard time managing my time lately.

@tomdale I agree that having "incomplete" records merged would be awesome and I could work on that, but from what I remember ember-data was supposed to not get any new features and focus on stability instead. If such a feature could still be sneaked into master before releasing stable version, though, I would love to help.

In Travis we use "incomplete" records in order to lower the load from the API. The idea is to allow using records which may not be a complete representation without doing additional requests do the API. Let's say you have a list of posts on a blog and to display an entry on the list you need title, publish date and author name. You would not want to push the body of the post through pusher, because it may be quite big and pusher data size is limited.

The solution is quite simple: create a record locally and set the data you get from pusher and load the rest of the data (ie. content) when someone tries to access it (for example when a user clicks on the post. The same needs to be done for accessing any relationships.

I've managed to implement it for Travis, but the implementation is outdated now and I ignore most of the issues with record states - most of the time we treat records as read-only on Travis.

The other thing which would greatly help is merge function, which would merge data updates into existing records. This is a requirement for incomplete records, so I guess we could start with pushing this first and then following up with incomplete records (or extended attributes).

Contributor
drogus commented Jun 28, 2013

Small update here. I'm switching Travis CI to use Ember Model and while discussing addition of such feature there, we came to conclusion that a lot of people have different expectations on how this feature should work. In consequence I ended up implementing only the hooks similar to the ones in this pull request. I think that this is the way to go for ember-data as well - as long as it's in early stages and it does not support some of the specific features, public tested extension points should be available for others to use.

The sad fact is that without such hooks and extension points there is no way to use the library if you have edge case scenarios not supported by the official version. I've used Ember Data for quite some time without too many problems (although we use mostly read only data), but every time I needed an upgrade I also had to rewrite all of my extensions, which relied on ED's internals. I was aware that it will be the case, every time you use internals, you will get bitten later, but I didn't really have a choice if I wanted to support things which the app needed.

@tomdale why don't you fell comfortable with such hooks? I know that once you add it, it will be hard to remove them, but as I mentioned above, I think that Ember Data should have good extension points for altering the default behavior. Rails was bitten by lack of such public APIs before the rewrite to Rails 3 and it was a valid concern, using plugins was risky most of the time. I guess this is also a topic for yet another discussion, ie. how extensible should ED be.

Owner
wagenet commented Aug 10, 2013

From what I've heard, @wycats is definitely planning on partial loading support.

Owner
wycats commented Aug 10, 2013

These hooks also seem like a good idea.

@wycats wycats was assigned Aug 10, 2013

@wagenet @wycats Is there any expected date of when this feature will be available? For the project we are working we need to make use of that feature and we do not know if we implement it on our own or wait for the official implementation.

Owner
wycats commented Sep 2, 2013

@drogus is it enough just to be notified that an attribute was requested, or do you need to be able to return something in that case? All attributes, or just undefined ones?

Owner
wagenet commented Oct 13, 2013

@drogus, ping

Contributor
drogus commented Oct 13, 2013

Sorry, I missed @wycats' comment.

Yes, it's enough to be notified, so this could also be an event I guess.

Owner
igorT commented May 21, 2014

This is super old, and I think there are some nicer api hooks now, so closing the PR. Partial record support is definitely something we will try and get to in the future.

@igorT igorT closed this May 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment