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

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

Merged
merged 7 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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'

Expand Down Expand Up @@ -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 }}
Expand Down Expand Up @@ -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
3 changes: 3 additions & 0 deletions addon/array/stateful.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
42 changes: 42 additions & 0 deletions tests/helpers/assertion.js
Original file line number Diff line number Diff line change
@@ -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;
}
6 changes: 4 additions & 2 deletions tests/test-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
12 changes: 6 additions & 6 deletions tests/unit/fragment_array_property_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
10 changes: 3 additions & 7 deletions tests/unit/fragment_owner_property_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/fragment_property_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/polymorphic_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/store_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
27 changes: 22 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4728,20 +4728,21 @@ ember-try-config@^4.0.0:
remote-git-tags "^3.0.0"
semver "^7.3.2"

ember-try@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/ember-try/-/ember-try-2.0.0.tgz#2671c221f5a0335fa2c189d00db7146e2e72a1dd"
integrity sha512-2N7Vic45sbAegA5fhdfDjVbEVS4r+ze+tTQs2/wkY+8fC5yAGHfCM5ocyoTfBN5m7EhznC3AyMsOy4hLPzHFSQ==
ember-try@^3.0.0:
version "3.0.0"
resolved "https://registry.yarnpkg.com/ember-try/-/ember-try-3.0.0.tgz#3b4e8511b64925aff14224b408fb5b5adab69391"
integrity sha512-ZYVKYWMnrHSD3vywo7rV76kPCOC9ATIEnGGG/PEKfCcFE0lB26jltRDnOrhORfLKq0JFp62fFxC/4940U+MwRQ==
dependencies:
chalk "^4.1.2"
cli-table3 "^0.6.0"
core-object "^3.1.5"
debug "^4.3.2"
ember-try-config "^4.0.0"
execa "^4.1.0"
fs-extra "^9.0.1"
fs-extra "^6.0.1"
resolve "^1.20.0"
rimraf "^3.0.2"
semver "^7.5.4"
walk-sync "^2.2.0"

emoji-regex@^7.0.1:
Expand Down Expand Up @@ -5777,6 +5778,15 @@ fs-extra@^5.0.0:
jsonfile "^4.0.0"
universalify "^0.1.0"

fs-extra@^6.0.1:
version "6.0.1"
resolved "https://registry.yarnpkg.com/fs-extra/-/fs-extra-6.0.1.tgz#8abc128f7946e310135ddc93b98bddb410e7a34b"
integrity sha512-GnyIkKhhzXZUWFCaJzvyDLEEgDkPfb4/TPvJCJVuS8MWZgoSsErf++QpiAlDnKFcqhRlm+tIOcencCjyJE6ZCA==
dependencies:
graceful-fs "^4.1.2"
jsonfile "^4.0.0"
universalify "^0.1.0"

fs-extra@^7.0.0, fs-extra@^7.0.1:
version "7.0.1"
resolved "https://registry.yarnpkg.com/fs-extra/-/fs-extra-7.0.1.tgz#4f189c44aa123b895f722804f55ea23eadc348e9"
Expand Down Expand Up @@ -9849,6 +9859,13 @@ semver@^7.0.0, semver@^7.2.1, semver@^7.3.2, semver@^7.3.4, semver@^7.3.5, semve
dependencies:
lru-cache "^6.0.0"

semver@^7.5.4:
version "7.6.0"
resolved "https://registry.yarnpkg.com/semver/-/semver-7.6.0.tgz#1a46a4db4bffcccd97b743b5005c8325f23d4e2d"
integrity sha512-EnwXhrlwXMk9gKu5/flx5sv/an57AkRplG3hTK68W7FRDN+k+OWBj65M7719OkA82XLBxrcX0KSHj+X5COhOVg==
dependencies:
lru-cache "^6.0.0"

send@0.18.0:
version "0.18.0"
resolved "https://registry.yarnpkg.com/send/-/send-0.18.0.tgz#670167cc654b05f5aa4a767f9113bb371bc706be"
Expand Down
Loading