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

Sync relationships clearing when links are present #5866

Closed
ryanto opened this issue Feb 22, 2019 · 4 comments
Closed

Sync relationships clearing when links are present #5866

ryanto opened this issue Feb 22, 2019 · 4 comments
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@ryanto
Copy link
Contributor

ryanto commented Feb 22, 2019

I think I'm running into some unexpected behavior with sync relationships that have links.

When I push a sync relationship with links into the store the relationship is emptied with a known-to-be-empty warning. If my model has related data, then that related data is being unset.

Reproduction

Here's a GH repo that shows a simplified version of what I'm seeing: https://github.com/ryanto/ember-data-sync-relationship-clear

I created a code sandbox for this repo: https://codesandbox.io/s/k9n7qz1k5r

Description

This behavior looks like it was changed in 3.2, on 3.1 the relationship is not cleared.

I think I would expect the known-to-be-empty warning to not be shown if Ember Data does know about the model having a relationship.

Versions

 ~/p/ember-data-sync-relationship-clear$ npm ls ember-source; npm ls ember-cli; npm ls ember-data                        

[email protected] /Users/ryan/projects/ember-data-sync-relationship-clear
└── [email protected]

[email protected] /Users/ryan/projects/ember-data-sync-relationship-clear
└── [email protected]

[email protected] /Users/ryan/projects/ember-data-sync-relationship-clear
└── [email protected]

Thanks!

@runspired
Copy link
Contributor

We restored a behavior true prior to 2.18 that we had unknowingly eliminated (hadn't been a guarantee and had no test coverage, basically wasn't a known case, just one that happened to exist a long time).

Basically, if a relationship is sync and you push a payload that has no data member at all, we end up needing to assume that it is empty.

I think what you are finding here is that this is at odds with being able to "update the links" associated with a relationship, although I am a bit surprised that a relationship might be established without a link and later receive one...

@ryanto
Copy link
Contributor Author

ryanto commented Feb 23, 2019

Thanks for getting back to so quickly, appreciate that!

Both of the payloads have links, but that payload that made a request with jsonapi includes also had data. The backend application I work on uses a json:api library that always includes links, for requests made with and without includes.

Assuming the relationship is empty makes sense to me, but I'm wondering if we could expand the logic a little. Would you accept a PR that did...

  1. If a sync relationship is not loaded and gets a payload with no data, it's assumed to be empty.
  2. If a sync relationship is empty and gets a payload with no data, it's assumed to be empty.
  3. If a sync relationship is not empty and gets a payload with no data, it's left unchanged.

Let me know, happy to tackle this one.

PS: Code sandbox crashes a lot with Ember apps - if the link it's working let me know and I can fix it (by rebooting the sandbox until it comes back :D )

@runspired
Copy link
Contributor

The proposed change seems good to me, especially if tests accompany it <3

@ryanto
Copy link
Contributor Author

ryanto commented Mar 9, 2019

Woohoo! I just updated the test app I was using to reproduce the original issue with emberjs/data#master and everything works as expected!

Closing this, see #5880

@ryanto ryanto closed this as completed Mar 9, 2019
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

No branches or pull requests

2 participants