Bring back support for loading embedded associations. #428

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
Contributor

mspisars commented Oct 19, 2012

both hasMany and belongsTo used to support embedded associations and got lost somewhere in the relationship-improvement branch.

Owner

wagenet commented Oct 20, 2012

@mspisars Was this the only code there was? I thought embedded associations were a lot more complex than just two lines.

Contributor

mspisars commented Oct 20, 2012

@wagenet this is all you need to load embedded associations... I guess my wording is a little misleading, but I will add code to this PR to account for serializing associations as well. currently that is in my custom/extended serializer, so I am in the process of moving it to serializer.js.

Stay tuned...

Owner

trek commented Oct 22, 2012

If you follow the code path here https://github.com/mspisars/data/blob/fde61a01123cb678a8bd4ab6f78de1526b6c10b8/packages/ember-data/lib/system/associations/has_many.js#L20 you'll find that this functionality does exist for hasMany, it's just been reorganized and made to work better with the new layers added on the relationship-improvement branch. d870297 is the relevant commit.

belongsTo still lacks the feature. It should be added with a similar pattern... also, tests and documentation.

Contributor

ppcano commented Oct 22, 2012

@trek, there is no mention of the embedded feature on master. emberjs/data#430

I think, you recommend to implement it on findAssociation adapter method, however i am not sure, if this should be done automatically by the adapter based on the serializer implementation. I am also confused why there is not a default implementation of the findAssociation method.

Owner

trek commented Oct 22, 2012

Embedded has since gone away, but will likely come back. As @mspisars clarified:

this is all you need to load embedded associations... I guess my wording is a little misleading,

this PR is really about loading associated data from the server. There's no default implementation of any of the Adapter methods: we have no way of knowing how your remote service is structured.

Contributor

mspisars commented Oct 23, 2012

looks like my VM clock is jacked up.
the merge d1f7c01 is my first stab at porting my custom stuff to the base serializer.

pex commented Oct 24, 2012

+1 to fix this - even though my embedded relations are still not loading after patching. People seem to be quite confused about this (e.g. a pull-requests that assumes an intention behind the removal #430).
How about test-coverage of embedded models?

Bringing back support for nested models would be great.
The reason for my pull request was to inform other people about the change before they try out the new version and wonder why it is not working even after applying all the other steps mentioned in the breaking changes.
Yes there is some confusion about this :-)

rickard2 commented Nov 1, 2012

Anyone know how work on bringing back embedded associations is prioritized? I'm currently building an app which uses embedded associations which means I'm stuck at rev 4 at the moment. If I need to refactor into not using embedded associations I'd like to know as soon as possible :)

Contributor

enyo commented Nov 3, 2012

I would like to have some update on that as well... @tomdale or @wycats could you comment on that? Is there any specific reason why this feature has been removed? Will it come back? If not: what is the right way to handle embedded records now?

I'm working with mongodb, so child documents are often embedded by nature and requesting them explicitly would create a lot of unnecessary code.

The slides from the last ember meetup indicate that they are working on embedded data. https://speakerdeck.com/tomdale/30?slide=31

Owner

trek commented Nov 4, 2012

If you need embedded objects immediately, you can extract the nested object at find/findAll time, assign them an id that is guaranteed unique, and add this to the association of the parent object:

var id = 1;
$.ajax({
  success: function(blogPostData){
    var comments = blogPostData.comments; // array of embedded comments
    blogPostData.comments = []; // new empty array for related ids

    comments.forEach(function(comment){
      comment.id = ++id; // new unique id
      blogPostData.comments.push(comment.id); // associate blog/comment for store
    });

    store.loadMany(comments); // load comments
    store.load(blogPostData); // load blog post
  }
})

Not ideal, but it will let you get started moving to newer revisions. You'll be able to strip this out entirely once embedded object support returns.

@mspisars mspisars * in trying to merge this to be helpful to someone that needs embedde…
…d associations, it looks like a missed a change to the DS.Model.dataDidChange() method...
bc52125

Omac commented Nov 5, 2012

I'm reopening models.

For a findAll request, and manual embedding a belongsTo you can quickly do this for the meen while:

App.Post.reopenClass( 
    findAll: () ->
        $.ajax('/posts',
            success: ((data) ->
                posts = data.posts
                for post in posts
                    App.store.load(App.Author, post.author.id, post.author)
                App.store.loadMany(App.Post, posts.mapProperty('id'), posts)
            )
        )
        App.store.all(App.Post)
)

App.Post.findAll()

I believe the clientId's must be unique regardless the type model, so make sure they are.

For a findQuery request i managed to solve it like this:

App.Comment.reopenClass(
    findQuery: (query) ->
        array = DS.AdapterPopulatedRecordArray.create( type: App.Comment, query: query, content: Ember.A([]), store: App.store)
        $.ajax('/comments',
            data: query                         
            success: ( (data) ->
                comments = data.comments
                # manually load any assosiation
                App.store.loadMany(App.Comment, comments.mapProperty('id'), comments);
                array.load(comments)
            )
        )
        array

App.Comment.findQuery(post_id: 1).addObserver('isLoaded', () ->
    # this.get('content')
)

Hope to see this feature supported soon.

pex commented Nov 11, 2012

@wagenet when do you think this is going to be evaluated?

Contributor

enyo commented Nov 11, 2012

Thanks @trek . That perfectly solves the problem temporarily.

Owner

wagenet commented Nov 11, 2012

@pex: @wycats and @tomdale are actively working on this though I do not have an ETA.

Owner

wycats commented Nov 12, 2012

@wagenet I would give it an ETA of a week or two.

Contributor

mspisars commented Nov 12, 2012

@pex, looks like my original PR missed DS.Model change for embedded relationships to work... bc52125
sorry only realized and found it now ... the 8 days ago commit is because my sandbox vm clock got out of sync and I forgot to check before committing.
FWIW, I have had embedded relationships in production - with this patch - for the last few weeks without any issues.

Contributor

mspisars commented Dec 2, 2012

This will be incompatible with the new embedded-relationships that are being implemented. So, it is only good for rev 6 (maybe 7) or earlier of BREAKING_CHANGES.md doc. Closing.

mspisars closed this Dec 2, 2012

Owner

tomdale commented Dec 3, 2012

Embedded loading is now back!

Contributor

seanabrahams commented Dec 3, 2012

👍

Contributor

mspisars commented Dec 4, 2012

@tomdale what about serializing back to the server?

Contributor

jefflab commented Dec 15, 2012

@tomdale Is embedded serialization still under development, or should we attempt to achieve it by implementing dirtyRecordsForAttributeChange, dirtyRecordsForBelongsToChange, and dirtyRecordsForHasManyChange methods?

Will the RESTSerializer eventually dirty the parent record for hasMany relationships based on the embedded: 'load' mapping?

Contributor

jefflab commented Dec 16, 2012

For anyone attempting to use a custom adapter and serializer to achieve serializing of embedded associations, I have posted a proof of concept to SO:

http://stackoverflow.com/questions/13897307/how-to-serialize-embedded-associations-in-ember-data

I would appreciate any feedback or better alternatives.

Due to the complexity of the solution above, my plan is to avoid using embedded associations until this feature is better supported.

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