Fix updating fragment arrays multiple times in the same runloop (#487)

* run CI in production environment

* upgrade ember-try

Fixes the following error:
Error: Cannot copy '../../../browserslist/cli.js' to a subdirectory of itself, '../../../browserslist/cli.js'.
    at /home/runner/work/ember-data-model-fragments/ember-data-model-fragments/node_modules/ember-try/node_modules/fs-extra/lib/copy/copy.js:210:21
    at FSReqCallback.oncomplete (fs.js:180:23)

ember-cli/ember-try#967

* upgrade to node.js 18

* fix store.find deprecation

* replace assert.throws with expectAssertion

* fix updating fragment array multiple times

* revert to node 14 and use `yarn --ignore-engines`

Author: dwickern

.github/workflows/ci.yml

@@ -77,7 +77,7 @@ jobs:
             ${{ runner.os }}-${{ matrix.node-version }}-
 
       - name: Install Dependencies
-        run: yarn install --frozen-lockfile
+        run: yarn install --frozen-lockfile --ignore-engines
         if: |
           steps.cache-dependencies.outputs.cache-hit != 'true'
 
@@ -121,7 +121,7 @@ jobs:
             ${{ runner.os }}-${{ matrix.node-version }}-
 
       - name: Install Dependencies
-        run: yarn install --frozen-lockfile
+        run: yarn install --frozen-lockfile --ignore-engines
         if: |
           steps.cache-dependencies.outputs.cache-hit != 'true'
 
@@ -163,7 +163,7 @@ jobs:
             ${{ runner.os }}-${{ matrix.node-version }}-
 
       - name: Install Dependencies
-        run: yarn install --no-lockfile --non-interactive
+        run: yarn install --no-lockfile --non-interactive --ignore-engines
 
       - name: Test
         run: yarn test:ember --launch ${{ matrix.browser }}
@@ -217,11 +217,16 @@ jobs:
             ${{ runner.os }}-${{ matrix.node-version }}-
 
       - name: Install Dependencies
-        run: yarn install --frozen-lockfile
+        run: yarn install --frozen-lockfile --ignore-engines
         if: |
           steps.cache-dependencies.outputs.cache-hit != 'true'
 
       - name: Test
         env:
           EMBER_TRY_SCENARIO: ${{ matrix.ember-try-scenario }}
         run: node_modules/.bin/ember try:one $EMBER_TRY_SCENARIO
+
+      - name: Test (Prod)
+        env:
+          EMBER_TRY_SCENARIO: ${{ matrix.ember-try-scenario }}
+        run: node_modules/.bin/ember try:one $EMBER_TRY_SCENARIO --- ember test --environment=production

addon/array/stateful.js

@@ -115,6 +115,9 @@ const StatefulArray = EmberObject.extend(MutableArray, Copyable, {
       // array is unchanged
       return;
     }
+    if (this._isDirty) {
+      this.retrieveLatest();
+    }
     const data = this.currentState.slice();
     data.splice(
       start,

package.json

@@ -67,7 +67,7 @@
     "ember-source": "~4.6.0",
     "ember-source-channel-url": "^3.0.0",
     "ember-template-lint": "^5.3.1",
-    "ember-try": "^2.0.0",
+    "ember-try": "^3.0.0",
     "eslint": "^7.32.0",
     "eslint-config-prettier": "^8.6.0",
     "eslint-plugin-ember": "^11.4.3",

tests/helpers/assertion.js

@@ -0,0 +1,42 @@
+import { getDebugFunction, setDebugFunction } from '@ember/debug';
+import { DEBUG } from '@glimmer/env';
+
+/**
+ * Asserts that `Ember.assert` is called with a falsy condition
+ * @param func function which calls `Ember.assert`
+ * @param expectedMessage the expected assertion text to compare with the first argument to `Ember.assert`
+ */
+function expectAssertion(func, expectedMessage) {
+  if (!DEBUG) {
+    this.ok(true, 'Assertions disabled in production builds');
+    return;
+  }
+  const originalAssertFunc = getDebugFunction('assert');
+  try {
+    let called = false;
+    let failed = false;
+    let actualMessage;
+    setDebugFunction('assert', function assert(desc, test) {
+      called = true;
+      if (!test) {
+        failed = true;
+        actualMessage = desc;
+      }
+    });
+    func();
+    this.true(called, `Expected Ember.assert to be called`);
+    this.true(failed, `Expected Ember.assert to fail its test`);
+    this.strictEqual(
+      actualMessage,
+      expectedMessage,
+      'Expected Ember.assert message to match'
+    );
+  } finally {
+    // restore original assert function
+    setDebugFunction('assert', originalAssertFunc);
+  }
+}
+
+export function setup(assert) {
+  assert.expectAssertion = expectAssertion;
+}

tests/test-helper.js

@@ -2,11 +2,13 @@ import Application from 'dummy/app';
 import config from 'dummy/config/environment';
 import * as QUnit from 'qunit';
 import { setApplication } from '@ember/test-helpers';
-import { setup } from 'qunit-dom';
+import { setup as setupQunitDom } from 'qunit-dom';
+import { setup as setupCustomAssertions } from './helpers/assertion';
 import { start } from 'ember-qunit';
 
 setApplication(Application.create(config.APP));
 
-setup(QUnit.assert);
+setupQunitDom(QUnit.assert);
+setupCustomAssertions(QUnit.assert);
 
 start();

tests/unit/fragment_array_property_test.js

@@ -195,11 +195,11 @@ module('unit - `MF.fragmentArray` property', function (hooks) {
     const person = await store.findRecord('person', 1);
     const addresses = person.addresses;
 
-    assert.throws(() => {
+    assert.expectAssertion(() => {
       const otherPerson = store.createRecord('person');
 
       addresses.addFragment(otherPerson);
-    }, 'error is thrown when adding a DS.Model instance');
+    }, "You can only add 'address' fragments or object literals to this property");
   });
 
   test('adding fragments from other records throws an error', async function (assert) {
@@ -212,9 +212,9 @@ module('unit - `MF.fragmentArray` property', function (hooks) {
     ]);
     const address = people[0].addresses.firstObject;
 
-    assert.throws(() => {
+    assert.expectAssertion(() => {
       people[1].addresses.addFragment(address);
-    }, 'error is thrown when adding a fragment from another record');
+    }, 'Fragments can only belong to one owner, try copying instead');
   });
 
   test('setting to an array of fragments is allowed', async function (assert) {
@@ -422,9 +422,9 @@ module('unit - `MF.fragmentArray` property', function (hooks) {
     pushPerson(1);
 
     const person = await store.findRecord('person', 1);
-    assert.throws(() => {
+    assert.expectAssertion(() => {
       person.set('addresses', ['address']);
-    }, 'error is thrown when setting to an array of non-fragments');
+    }, "You can only add 'address' fragments or object literals to this property");
   });
 
   test('fragments can have default values', function (assert) {

tests/unit/fragment_owner_property_test.js

@@ -52,13 +52,9 @@ module('unit - `MF.fragmentOwner` property', function (hooks) {
 
     const invalid = store.createRecord('invalidModel');
 
-    assert.throws(
-      () => {
-        invalid.owner;
-      },
-      /Fragment owner properties can only be used on fragments/,
-      'getting fragment owner on non-fragment throws an error'
-    );
+    assert.expectAssertion(() => {
+      invalid.owner;
+    }, 'Fragment owner properties can only be used on fragments.');
   });
 
   test("attempting to change a fragment's owner record throws an error", async function (assert) {

tests/unit/fragment_property_test.js

@@ -111,9 +111,9 @@ module('unit - `MF.fragment` property', function (hooks) {
     });
 
     const person = await store.findRecord('person', 1);
-    assert.throws(() => {
+    assert.expectAssertion(() => {
       person.set('name', store.createRecord('person'));
-    }, 'error is thrown when setting non-fragment');
+    }, 'You must pass a fragment or null to set a fragment');
   });
 
   test('setting fragments from other records throws an error', async function (assert) {
@@ -142,9 +142,9 @@ module('unit - `MF.fragment` property', function (hooks) {
       store.findRecord('person', 1),
       store.findRecord('person', 2),
     ]);
-    assert.throws(() => {
+    assert.expectAssertion(() => {
       people[1].set('name', people[0].name);
-    }, 'error is thrown when setting to a fragment of another record');
+    }, 'Fragments can only belong to one owner, try copying instead');
   });
 
   test('null values are allowed', async function (assert) {

tests/unit/polymorphic_test.js

@@ -94,7 +94,7 @@ module('unit - Polymorphism', function (hooks) {
       },
     });
 
-    const record = await store.find('zoo', 1);
+    const record = await store.findRecord('zoo', 1);
     const animals = record.animals;
 
     const newLion = animals.createFragment({
@@ -138,7 +138,7 @@ module('unit - Polymorphism', function (hooks) {
       },
     });
 
-    const component = await store.find('component', 1);
+    const component = await store.findRecord('component', 1);
     const textOptions = component.optionsHistory.createFragment({
       fontFamily: 'Verdana',
       fontSize: 12,

tests/unit/store_test.js

@@ -29,9 +29,9 @@ module('unit - `DS.Store`', function (hooks) {
   });
 
   test('attempting to create a fragment type that does not inherit from `MF.Fragment` throws an error', function (assert) {
-    assert.throws(() => {
+    assert.expectAssertion(() => {
       store.createFragment('person');
-    }, 'an error is thrown when given a bad type');
+    }, "The 'person' model must be a subclass of MF.Fragment");
   });
 
   test('the store has an `isFragment` method', function (assert) {