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

unload should invalidate async hasMany: failing test #5353

Closed
wants to merge 1 commit into from

Conversation

jlami
Copy link
Contributor

@jlami jlami commented Feb 14, 2018

This simulates the problems I'm having when trying to debug #5343

As far as I could tell the new behaviour for async relationships is not to mark unloaded items as deleted, but to use invalidate+refetch as mentioned here: 6e8a236
But I'm having trouble understanding whether the invalidate should be done by the ember-data user, or by ember-data.
I feel this should be ember-data's job, but this test has one line in comment where the user could hasMany(....).reload() to refetch the data to fix the problem. But that feels like a bad solution.

@jlami
Copy link
Contributor Author

jlami commented Feb 14, 2018

Might be related to #5328

@jlami
Copy link
Contributor Author

jlami commented Feb 16, 2018

@sly7-7 I think the problem here is partly due to me not understanding the new async handling. The first assert of the count not being correct after the unload could probably be ignored. But the final exception might be the real problem.

Where ember-data now correctly is able to reload an dematerialized record that is still in the ManyArray, it fails to handle the case when a record is deleted on the server side. The rejected fetch will result in a complete failure now, while this could actually be a valid response. So maybe this test should be renamed into 'async relationship can handle server side deletion' or something like that?

@jlami
Copy link
Contributor Author

jlami commented Feb 20, 2018

Does anybody have any idea on how a rejection in a loading ManyArray should be handled in async relationships? The async part makes it valid to have changes on the server side once the data is asked for. So I think ember-data should play well with any record that is rejected.

The findMany part only gives warnings if you don't include deleted items as far as I can tell: https://github.com/emberjs/data/blob/master/addon/-private/system/store.js#L892
But this path is only used if coalesceRecords is true I believe.

We don't coalesce records in our plugin and this path seems to throw exceptions that are not caught and I'm not really sure if this will actually update the ManyArray if it would be used with a model.get('manyArray').catch(...). And even if this would work, computed properties don't work this way. So I really think the ManyArray should handle this when reloading. Maybe here: https://github.com/emberjs/data/blob/master/addon/-private/system/relationships/state/has-many.js#L290

hjdivad added a commit to hjdivad/data that referenced this pull request Mar 13, 2018
Invalidates link promises when inverse records are unloaded.  Subsequent
fetches of a `hasMany` unloaded in this way will load from the link
again, rather than loading whatever ids were returned from the prior
link load.

[fix emberjs#5353]
@runspired
Copy link
Contributor

@bmac we can close this as fixed by #5377

@bmac bmac closed this Mar 16, 2018
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

Successfully merging this pull request may close these issues.

3 participants