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

Empty async many relationship when saving in Beta 10 #2339

Closed
jasalguero opened this issue Oct 2, 2014 · 16 comments
Closed

Empty async many relationship when saving in Beta 10 #2339

jasalguero opened this issue Oct 2, 2014 · 16 comments

Comments

@jasalguero
Copy link
Contributor

Related to #2331. Saving behaviour seems to have changed from Beta 9 and it looks wrong:

Having two model Group and User, with Group having many users.

The behaviour in Beta 9 was:
1- When a Group is saved, Ember Data would send the array of user ids to the server
2- When the sever replied with a links property, Ember data would trigger a request to get the dependent object and update if there is any change.

screen shot 2014-10-02 at 13 41 19

The behaviour in Beta10 is:
1- When a Group is saved, Ember Data sends an empty array as user ids to the server.
2- When the sever replied with a links property, Ember Data doesn't trigger any further request, ignoring any changes that can may have happened in the server

screen shot 2014-10-02 at 13 43 49

@igorT Unless the way the saving has to be done has changed I would say this seems like a serious bug for both differences, but specially the first one since is easy to overlook and will mess with the data of existing apps.

http://emberjs.jsbin.com/jepuno/2/

@igorT
Copy link
Member

igorT commented Oct 2, 2014

This was on purpose, I thought we shouldn't be refetching if the link didn't change. I am now thinking I might have been wrong. Any thoughts @wycats @tomdale ?

@jasalguero
Copy link
Contributor Author

I can see some logic on that, but I would say that that behaviour can lead to inconsistencies between server and client like in the jsbin after hitting save. And an extra request is worth saving a big pain down in the road. Maybe that could be optedIn/Out like the coalesce?

About the first issue, the sending of the empty id list, anything can be done for now?

@igorT
Copy link
Member

igorT commented Oct 2, 2014

I just tested with your jsbin, the first issue is already fixed on master

@jasalguero
Copy link
Contributor Author

Cool, would Beta 10 be patched or be part of the next one?

@igorT
Copy link
Member

igorT commented Oct 2, 2014

You can use canary or wait for 11 which should be in next few days

@jasalguero
Copy link
Contributor Author

Great, thanks for blazing fast answer! Maybe it would be good to put a note in the changes md of Beta 10 about it. I know I would have been very sorry if I had hit prod with this bug...

@igorT
Copy link
Member

igorT commented Oct 2, 2014

PR welcome

@sly7-7
Copy link
Contributor

sly7-7 commented Oct 2, 2014

Actually I think that beta.10 should not be available in the builds page, because it has some subtle bugs like this.
Anyway, concerning the second point, for now you can manually call reload on the hasMany relationship. Thinking about doing this automatically, I'm pretty sure some people want it, and some people don't want it.

@jasalguero
Copy link
Contributor Author

Sure, I'll make the note in the changelog and do a PR.

@igorT do you know the commit where this issue was fixed?
Since there the tags don't have a branch I will create a new branch at the tag's head and will push it there, so the tag can be recreated based on that branch.

@sly7-7 I agree with you about removing the build, but I think it should also be documented somewhere, since it's been already more than 2 weeks the beta has been around.

@igorT
Copy link
Member

igorT commented Oct 2, 2014

cc @fivetanley

@jasalguero
Copy link
Contributor Author

PR created: #2340

@MichaelVdheeren
Copy link

When can we expect a new build to be released with this? And in respect to that, what's up with the new release schedule proposed about a month ago (similar to ember.js itself)?

@igorT
Copy link
Member

igorT commented Oct 6, 2014

CC our build/release master @fivetanley

@emorling
Copy link

emorling commented Oct 6, 2014

+subscribe, I am also looking forward to Beta 11 for this fix

@MichaelVdheeren
Copy link

@fivetanley What's that status of a new build which works with latest ember.js beta?

@igorT
Copy link
Member

igorT commented Oct 16, 2014

beta 11 is out

@igorT igorT closed this as completed Oct 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants