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

pushPayload does not change hasMany relationship when going to empty state #4804

Closed
danielspaniel opened this issue Feb 15, 2017 · 10 comments
Closed

Comments

@danielspaniel
Copy link

danielspaniel commented Feb 15, 2017

I have a model which hasMany things, then I pushPayload with data that says .. I have no things anymore. Ember data ignores that and keeps the hasMany things.

The key activities are that:

  • I have a model with no things
  • then I set some things model.set('things', things)
  • then I pushPayload to remove them.

Here is a twiddle to show the problem:

https://ember-twiddle.com/26204b7fa7fc185dde464418cd33df95?openFiles=routes.application.js%2C

@hjdivad
Copy link
Member

hjdivad commented Feb 16, 2017

@danielspaniel https://ember-twiddle.com/53d305379774c4756d576107c98ae6d4?openFiles=routes.application.js%2C

In the above twiddle you can comment / uncomment line 48 (ie the this.store.pushPayload line) to see things either cleared or not cleared.

There are two changes as compared to your original twiddle.

  1. the initial relationships data is populated, the relationships are not merely sieloaded in the included payload
  2. the manyarray itself is used and not overwritten by the live array from peekAll (ie the person.set('things', things) line is commented).

Please let me know if I have misunderstood the issue your twiddle aimed to highlight, but I believe the core error in the above was in the payload and not an ember data bug.

@runspired
Copy link
Contributor

The issue with the first twiddle is that the relationship is being added as "dirty state", it won't be cleared because no save attempt has been made (so obviously a server update would not know about it).

@danielspaniel
Copy link
Author

danielspaniel commented Feb 16, 2017

The reason I flag this as a bug is because even in a dirty state if you pushPayload with values for the hasMany then the relationship will update itself to add those new rows.
By your logic in a "dirty state" relationship pushing data to a relationship would always have no affect.

Here is a gist to show what I mean:
https://ember-twiddle.com/983bf2f8ac50eaf7c5f1ffd3bdc6839b?openFiles=routes.application.js%2C

But I know what your going to say ... Hey Dan, you see, the start state is empty, you set some things, then pushPayload tries to clear them, but since this hasMany does not "know" about those 2 new things, it can't remove them.

2 flaws in that argument.
The biggest flaw: pushPayload resets the state of the model to root.loaded.saved, so in essence you now can not even save those 2 new things, because the model thinks. Ok I am all good.

Also, the belongsTo relationship is happy to clear any "dirty state" ( new model ) you set when you pushPayload.

So, it seems like pushPayload is just not quite 100% clear on it's mission for what to do with hasMany because usually pushPayload just sets the state to what you pushed, no matter what state your in .. no questions asked, and resets the state of the model to root.loaded.saved. It is a way to update the model immediately to a new state, and call it saved. Which works for 95% of cases except the hasMany.

@Exelord
Copy link

Exelord commented Mar 16, 2017

Any news here?

@runspired
Copy link
Contributor

@Exelord mostly fixed. There's one case for "current vs canonical" where we get it wrong that has not been fixed which is this test PR: #5269 and is the case mentioned by @danielspaniel in his last comment. Once the RecordData RFC lands I will be doing a pass through the relationship layer making things nice.

@runspired
Copy link
Contributor

I believe #4852 improves this situation, once that lands I will add more tests and hopefully this issue will be resolved :)

@Dirk-27
Copy link

Dirk-27 commented Jun 4, 2019

I ran into the same issue with ember-data Version 3.8: empty relationship arrays in the payload didn't clear the relationship. What is the state of this issue?

@wuarmin
Copy link

wuarmin commented Jul 13, 2020

I have the same issue

@runspired
Copy link
Contributor

@Dirk-27 the state of this issue is there is nothing more we can do for push api's. It is equally incorrect to discard local mutations as it is to not discard them: it just boils down to what your app wants.

While not explicitly enabled by the new cache design in emberjs/rfcs#854 once @ember-data/graph is made public rollback will be a public API. Custom cache implementations can also choose today to do one behavior vs the other.

@runspired
Copy link
Contributor

Also closing this as at this point this request is more about relationship-rollback than it is about updating the state. The store forking behavior introduced in emberjs/rfcs#854 allows applications to better choose whether to display remote or mutated state as desired, so rollback and this issue are dead artifacts of a previous era of APIs.

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

6 participants