[BUGFIX beta] prevent record duplication due to EmbeddedRecordsMixin #4441
Conversation
|
This is my first try at fixing this issue that has been haunting our software at times. Please let me know what you guys think. :) |
addon/-private/system/store.js
Outdated
| types.forEach(function (type) { | ||
| var existingRecords = store.peekAll(type); | ||
| var uncommitted = existingRecords.filter(function (record) { | ||
| return record.get('isNew') && !record.get('isSaving'); |
There was a problem hiding this comment.
Looks like this will unload all record of a type the isNew state even if they were not the records sent in the original request?
There was a problem hiding this comment.
correct. It does. There currently is no test that fails showcasing that scenario, but it's easy to write one.
Problem is, I am stumbling with a way to let the store know which records are being persisted to the backend. Should the EmbeddedRecordMixin flag the records by changing their currentState to 'inFlight'? How should I handle this in the Ember data runloop?
|
Ok. So I added a test that fails due to my current implementation. Can anyone offer some advice on how to improve it? |
|
@igorT I think you had some ideas on how handle this? |
|
|
||
| run(function() { | ||
| post = store.createRecord('post', { name: 'The Parley Letter' }); | ||
| post2 = store.createRecord('post', { name: 'Another Parley Letter' }); |
There was a problem hiding this comment.
It doesn't look like post2 is defined above on line 2643 like post.
There was a problem hiding this comment.
yeah, the linter fails 'cause of that as well. It has no bearing on the failing of this test, however.
tarzan
force-pushed
the
embedded-duplications-fix
branch
from
3140ba9
to
ecbf416
Compare
|
@jcbvm correct. That's why there still is a failing spec. I am still looking for suggestions on how to handle this. Problem is that the |
… proposed fix to emberjs#1829 When the EmbeddedRecordsMixin is used to persist embedded records to the backend, the parent record is handled as expected. However, nested children are neglected in the run loop and upon pushing a returned payload from the server into the store we end up with duplication. This commits adds a procedure for cleaning up these flagged 'uncommitted' records from the store before pushing the created ones.
tarzan
force-pushed
the
embedded-duplications-fix
branch
from
ecbf416
to
33df55e
Compare
|
Ok. I changed it around a bit. Now the embedding of a (hasmany) record that has not yet persisted, will get a dummy 'embedded' ID. As a result, the store will be able to find these records when processing the payload. To me, this implementation seems a bit hacky, but I'm having a hard time coming up with a different approach. Any feedback will be greatly appreciated! @igorT how does it differ from the ideas you have? |
|
Any news on this? |
|
The duplication issue can be resolved via use of |
This is a proposed fix to #1829
When the EmbeddedRecordsMixin is used to persist embedded records to the backend, the parent record is handled as expected. However, nested children are neglected in the run loop and upon pushing a returned payload from the server into the store we end up with duplication. This commits adds a procedure for cleaning up these flagged 'uncommitted' records from the store before pushing the created ones.