-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: record.destroyRecord should also unload the record #7258
Conversation
Asset Size Report for 053aaf4 IE11 Builds 🛑 The size of the library EmberData has increased by +5.31 KB (+860.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsChangeset
Full Asset Analysis (IE11)
Modern Builds 🛑 The size of the library EmberData has increased by +4.63 KB (+845.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsChangeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) 🛑 The size of the library EmberData has increased by +5.19 KB (+885.0 B compressed) which exceeds the failure threshold of 75 bytes.WarningsChangeset
Full Asset Analysis (Modern)
|
packages/-ember-data/tests/acceptance/relationships/belongs-to-test.js
Outdated
Show resolved
Hide resolved
Performance Report for 053aaf4 Scenario - materialization: ✅ Performance improved
Scenario - unload: ✅ Performance improved
Scenario - destroy:
|
|
20c4aad
to
73583ec
Compare
4ef95c0
to
167b6c2
Compare
edb8c95
to
0dc0f4f
Compare
We addressed the first failure in The other test failure seems to be a change in the way I'm not sure why we're looking at |
@jrjohnson I'd love to investigate that with you before you remove it, we may need to fix a notify somewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall approved but we need to fix M3 (involves just going back to setting currentState on the model in the FF off branch) / potential issue with ilios (potential lack of notification on isNew)
d417dcd
to
2bfded0
Compare
db37402
to
e0614a9
Compare
85f7d51
to
6aca536
Compare
Things left to do: |
Sure but this method calls unload which is how you end up with a full
deletion. No need to have the outer wrapper here, as the call to unload
record should take care of it
…On Mon, May 10, 2021, 22:24 Chris Thoburn ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/store/addon/-private/system/model/internal-model.ts
<#7258 (comment)>:
> this.currentState = RootState.empty;
if (REMOVE_RECORD_ARRAY_MANAGER_LEGACY_COMPAT) {
this.store.recordArrayManager.recordDidChange(this.identifier);
}
}
deleteRecord() {
- if (RECORD_DATA_STATE) {
- if (this._recordData.setIsDeleted) {
- this._recordData.setIsDeleted(true);
- }
- }
- this.send('deleteRecord');
+ run(() => {
(its a mess but deleteRecord / unloadRecord / rollbackAttributes all
result in a full deletion for newly created records)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7258 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFOTJ63S3MGUT4OTTUQ2WDTNCPOZANCNFSM4PQ3XHWA>
.
|
978923b
to
9a56cf9
Compare
… be derived Co-Authored-By: scott-newcomer <[email protected]>
9a56cf9
to
2264bb2
Compare
close #5006
close #5455
Port of changes from #7217