Add a note about the removed embedded option to BREAKING_CHANGES #430

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants

I guess this will break a lot of projects and currently it is not mentioned in BREAKING_CHANGES.

Contributor

mspisars commented Oct 23, 2012

but that is not the intended functionality.

+1

crofty commented Nov 2, 2012

I am using embedded associations and have just been bitten by that fact that loading them doesn't work anymore.

It would have saved me a lot of time if this had been mentioned in the breaking changes file.

Contributor

klaaspieter commented Nov 19, 2012

Agreed. I also just spent an entire morning trying to figure out why it didn't work.

Doesn't it? Is there any better solution or is there any conceptual mistake about embedding records?

Contributor

mspisars commented Nov 20, 2012

No mistake, they are working on adding it back, in fact the first commit to bring this back was merged several days ago: 92e310e
My code attached to PR #428, is really a very small hack to get me the functionality.

Oh, awesome! :)

Contributor

marktheunissen commented Nov 27, 2012

I just spent a lot of time manually stepping through the code trying to figure out why this doesn't work ... so +1 for this PR from me, can save people a lot of wasted effort.

Contributor

workmanw commented Nov 27, 2012

Actually I think this was recently added back to ember-data. See 92e310e

Contributor

ivanvanderbyl commented Dec 2, 2012

Confirmed, this functionality appears to be on master again. If you have spent all morning pulling your hair out, make sure you're defining it as a mapping and not associations options as per https://github.com/emberjs/data/blob/master/BREAKING_CHANGES.md#mapping

e.g.

DS.RESTAdapter.map('App.Project', {
  tasks: { embedded: 'load' }
});
Contributor

workmanw commented Dec 2, 2012

Awesome. I think we can probably close this issue then? CC: @tomdale @NilsLattek

It is not fully implemented yet. Take a look at revision 9 in Breaking_Changes: "This strategy served us well, until we came to the case of embedded records (which we are working on, but have not yet finished)."
Indeed it is good to see it coming back soon and then there will be a new revision explaining the changes, so I guess I should close this PR.

NilsLattek closed this Dec 2, 2012

Owner

tomdale commented Dec 3, 2012

Very sorry about all of the frustration this caused! We messed up and forgot about adding this to the Breaking Changes document, due to the sheer scope of the relationship-improvements branch. It became a much bigger change than we anticipated and we were overwhelmed by the scope of the changes.

That being said, embedded loading is now back in, and we are actively working on embedded saving. The API is as @ivanvanderbyl described above.

I sincerely apologize that we screwed up on this and will happily buy you a beer or other beverage as penance if you were affected by this and we meet in person. :)

Contributor

ivanvanderbyl commented Dec 4, 2012

Hey @tomdale thanks for weighing in. I was attempting to upgrade ember data in my app over the weekend and got most of the original behavior back, however belongsTo associations seem to be behaving strangely when embedded, so would it be correct to assume there is still work to be done here? (I haven't been watching the commits for a few weeks)

I'm searching through issues now to find if anyone else is seeing the behavior I'm seeing, otherwise I'll lodge an issue.

Owner

tomdale commented Dec 4, 2012

@ivanvanderbyl belongsTo embedded loading should be working fine. This does require that the embedded record have an ID, and any changes to the embedded record will be saved like normal.

If you are seeing a bug, would you mind putting together a reduced unit test for us? Thanks!

Contributor

ivanvanderbyl commented Dec 4, 2012

Thanks will do tonight.

The behavior I'm seeing is the belongsTo association isn't materialized unless the parent also contains the associations ID, which then has the unintended side effect of loading the association again. I will try to isolate this outside of my app and put together a fiddle.

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