[BUGFIX] destroyRecord should also unload

addon/-legacy-private/system/model/internal-model.js

@@ -5,7 +5,7 @@ import EmberError from '@ember/error';
 import { isEqual } from '@ember/utils';
 import { setOwner } from '@ember/application';
 import { run } from '@ember/runloop';
-import RSVP, { Promise } from 'rsvp';
+import { resolve, Promise, defer } from 'rsvp';
 import Ember from 'ember';
 import { DEBUG } from '@glimmer/env';
 import { assert, inspect } from '@ember/debug';
@@ -133,6 +133,7 @@ export default class InternalModel {
     this._promiseProxy = null;
     this._record = null;
     this._isDestroyed = false;
+    this.isDestroying = false;
     this.isError = false;
     this._pendingRecordArrayManagerFlush = false; // used by the recordArrayManager
 
@@ -426,12 +427,31 @@ export default class InternalModel {
   }
 
   deleteRecord() {
-    this.send('deleteRecord');
+    if (this.isNew()) {
+      // deleteRecord will mark us as `root.deleted.saved` which is not new
+      // this could be replaced by updating the state machine to transition
+      //   records that persist their deletion and are then unloaded into
+      //   root.deleted.saved.empty or root.deleted.empty
+      //   since we know these records no longer exist remotely and should not
+      //   be fetched again.
+      this._deletedRecordWasNew = true;
+      run(() => {
+        this.send('deleteRecord');
+        this._triggerDeferredTriggers();
+        this.unloadRecord();
+      });
+    } else {
+      this.send('deleteRecord');
+    }
   }
 
   save(options) {
+    if (this._deletedRecordWasNew) {
+      return resolve(null);
+    }
+
     let promiseLabel = 'DS: Model#save ' + this;
-    let resolver = RSVP.defer(promiseLabel);
+    let resolver = defer(promiseLabel);
 
     this.store.scheduleSave(this, resolver, options);
     return resolver.promise;
@@ -1197,12 +1217,13 @@ export default class InternalModel {
     @method adapterDidCommit
   */
   adapterDidCommit(data) {
+    let store = this.store;
+
     if (data) {
-      this.store._internalModelDidReceiveRelationshipData(
-        this.modelName,
-        this.id,
-        data.relationships
-      );
+      // normalize relationship IDs into records
+      store.updateId(this, data);
+      store._setupRelationshipsForModel(this, data);
+      store._internalModelDidReceiveRelationshipData(this.modelName, this.id, data.relationships);
 
       data = data.attributes;
     }

addon/-legacy-private/system/model/model.js

@@ -1,6 +1,7 @@
 import ComputedProperty from '@ember/object/computed';
 import { isNone } from '@ember/utils';
 import EmberError from '@ember/error';
+import { run } from '@ember/runloop';
 import Evented from '@ember/object/evented';
 import EmberObject, { computed, get } from '@ember/object';
 import Map from '../map';
@@ -66,7 +67,7 @@ function intersection(array1, array2) {
 const RESERVED_MODEL_PROPS = ['currentState', 'data', 'store'];
 
 const retrieveFromCurrentState = computed('currentState', function(key) {
-  return get(this._internalModel.currentState, key);
+  return this._internalModel.currentState[key];
 }).readOnly();
 
 /**
@@ -614,7 +615,12 @@ const Model = EmberObject.extend(Evented, {
   */
   destroyRecord(options) {
     this.deleteRecord();
-    return this.save(options);
+    return this.save(options).then(() => {
+      // the nested runloop here is necessary to ensure that the record is fully
+      //   destroyed prior to the promise resolving.
+      //   run.join is inadequate as the destroy queue would still be flushed after the resolve
+      run(() => this.unloadRecord());
+    });
   },
 
   /**

addon/-legacy-private/system/model/states.js

@@ -699,6 +699,8 @@ const RootState = {
         internalModel.triggerLater('didCommit', internalModel);
       },
 
+      // TODO @runspired should we special case unloadRecord for root.deleted.saved ?
+
       willCommit() {},
       didCommit() {},
       pushedData() {},

addon/-legacy-private/system/relationships/state/belongs-to.js

@@ -233,6 +233,7 @@ export default class BelongsToRelationship extends Relationship {
    */
   getData(isForcedReload = false) {
     //TODO(Igor) flushCanonical here once our syncing is not stupid
+    // TODO @runspired I suspect our syncing is now "good enough"™
     let record = this.inverseInternalModel ? this.inverseInternalModel.getRecord() : null;
 
     if (this.shouldMakeRequest()) {

addon/-legacy-private/system/relationships/state/has-many.js

@@ -346,6 +346,7 @@ export default class ManyRelationship extends Relationship {
 
   getData(isForcedReload = false) {
     //TODO(Igor) sync server here, once our syncing is not stupid
+    // TODO @runspired I suspect our syncing is now "good enough"™
     let manyArray = this.manyArray;
 
     if (this.shouldMakeRequest()) {

addon/-legacy-private/system/store.js

@@ -2046,20 +2046,14 @@ Store = Service.extend({
     if (dataArg) {
       data = dataArg.data;
     }
-    if (data) {
-      // normalize relationship IDs into records
-      this.updateId(internalModel, data);
-      this._setupRelationshipsForModel(internalModel, data);
-    } else {
-      assert(
-        `Your ${
-          internalModel.modelName
-        } record was saved to the server, but the response does not have an id and no id has been set client side. Records must have ids. Please update the server response to provide an id in the response or generate the id on the client side either before saving the record or while normalizing the response.`,
-        internalModel.id
-      );
-    }
 
-    //We first make sure the primary data has been updated
+    assert(
+      `Your ${
+        internalModel.modelName
+      } record was saved to the server, but the response does not have an id and no id has been set client side. Records must have ids. Please update the server response to provide an id in the response or generate the id on the client side either before saving the record or while normalizing the response.`,
+      internalModel.id || (data && data.id)
+    );
+
     //TODO try to move notification to the user to the end of the runloop
     internalModel.adapterDidCommit(data);
   },

addon/-record-data-private/system/model/internal-model.js

@@ -4,7 +4,7 @@ import { A } from '@ember/array';
 import { setOwner } from '@ember/application';
 import { run } from '@ember/runloop';
 import { assign } from '@ember/polyfills';
-import RSVP, { Promise } from 'rsvp';
+import { defer, resolve, Promise } from 'rsvp';
 import Ember from 'ember';
 import { DEBUG } from '@glimmer/env';
 import { assert, inspect } from '@ember/debug';
@@ -352,12 +352,31 @@ export default class InternalModel {
   }
 
   deleteRecord() {
-    this.send('deleteRecord');
+    if (this.isNew()) {
+      // deleteRecord will mark us as `root.deleted.saved` which is not new
+      // this could be replaced by updating the state machine to transition
+      //   records that persist their deletion and are then unloaded into
+      //   root.deleted.saved.empty or root.deleted.empty
+      //   since we know these records no longer exist remotely and should not
+      //   be fetched again.
+      this._deletedRecordWasNew = true;
+      run(() => {
+        this.send('deleteRecord');
+        this._triggerDeferredTriggers();
+        this.unloadRecord();
+      });
+    } else {
+      this.send('deleteRecord');
+    }
   }
 
   save(options) {
+    if (this._deletedRecordWasNew) {
+      return resolve();
+    }
+
     let promiseLabel = 'DS: Model#save ' + this;
-    let resolver = RSVP.defer(promiseLabel);
+    let resolver = defer(promiseLabel);
 
     this.store.scheduleSave(this, resolver, options);
     return resolver.promise;

addon/-record-data-private/system/model/model.js

@@ -3,6 +3,7 @@ import { isNone } from '@ember/utils';
 import EmberError from '@ember/error';
 import Evented from '@ember/object/evented';
 import EmberObject, { computed, get } from '@ember/object';
+import { run } from '@ember/runloop';
 import Map from '../map';
 import { DEBUG } from '@glimmer/env';
 import { assert, warn } from '@ember/debug';
@@ -615,7 +616,12 @@ const Model = EmberObject.extend(Evented, {
   */
   destroyRecord(options) {
     this.deleteRecord();
-    return this.save(options);
+    return this.save(options).then(() => {
+      // the nested runloop here is necessary to ensure that the record is fully
+      //   destroyed prior to the promise resolving.
+      //   run.join is inadequate as the destroy queue would still be flushed after the resolve
+      run(() => this.unloadRecord());
+    });
   },
 
   /**
@@ -790,9 +796,12 @@ const Model = EmberObject.extend(Evented, {
     successfully or rejected if the adapter returns with an error.
   */
   save(options) {
+    return this._internalModel.save(options).then(() => this);
+    /*
     return PromiseObject.create({
       promise: this._internalModel.save(options).then(() => this),
     });
+    */
   },
 
   /**

addon/-record-data-private/system/model/states.js

@@ -697,6 +697,8 @@ const RootState = {
         internalModel.triggerLater('didCommit', internalModel);
       },
 
+      // TODO @runspired should we special case unloadRecord for root.deleted.saved ?
+
       willCommit() {},
       didCommit() {},
       pushedData() {},

package.json

@@ -92,7 +92,7 @@
     "ember-qunit": "^3.4.1",
     "ember-qunit-assert-helpers": "^0.2.1",
     "ember-resolver": "^5.0.1",
-    "ember-source": "~3.3.0",
+    "ember-source": "2.18",
     "ember-source-channel-url": "^1.1.0",
     "ember-try": "^1.0.0-beta.3",
     "eslint": "^5.3.0",

tests/integration/adapter/rest-adapter-test.js

@@ -1097,32 +1097,36 @@ test('deleteRecord - a payload with sidloaded updates pushes the updates when th
     });
 });
 
-test('deleteRecord - deleting a newly created record should not throw an error', function(assert) {
+test('deleteRecord - deleting a newly created record should not throw an error', async function(assert) {
   let post = store.createRecord('post');
+  let internalModel = post._internalModel;
 
-  return run(() => {
-    post.deleteRecord();
-    return post.save().then(post => {
-      assert.equal(
-        passedUrl,
-        null,
-        'There is no ajax call to delete a record that has never been saved.'
-      );
-      assert.equal(
-        passedVerb,
-        null,
-        'There is no ajax call to delete a record that has never been saved.'
-      );
-      assert.equal(
-        passedHash,
-        null,
-        'There is no ajax call to delete a record that has never been saved.'
-      );
+  post.deleteRecord();
 
-      assert.equal(post.get('isDeleted'), true, 'the post is now deleted');
-      assert.equal(post.get('isError'), false, 'the post is not an error');
-    });
-  });
+  await post.save();
+
+  assert.equal(
+    passedUrl,
+    null,
+    'There is no ajax call to delete a record that has never been saved.'
+  );
+  assert.equal(
+    passedVerb,
+    null,
+    'There is no ajax call to delete a record that has never been saved.'
+  );
+  assert.equal(
+    passedHash,
+    null,
+    'There is no ajax call to delete a record that has never been saved.'
+  );
+
+  // destroyed objects do not allow computed properties to be accessed
+  //   so to check this state we need to go through the internalModel.
+  assert.equal(internalModel.currentState.isEmpty, true, 'the post is now deleted');
+  // TODO ideally we would transition to some sort of `root.deleted.empty` or `root.deleted.unloaded` form of state
+  // assert.equal(internalModel.currentState.isDeleted, true, 'the post is now deleted');
+  // assert.equal(internalModel.currentState.isError, false, 'the post is not an error');
 });
 
 test('findAll - returning an array populates the array', function(assert) {