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

Conversation

dwickern
Copy link
Contributor

@dwickern dwickern commented May 3, 2024

In production builds, certain fragment array mutations produce the wrong result. For example, this code swaps the first two items in the array:

const first = fragmentArray.objectAt(0)
fragmentArray.removeAt(0)
fragmentArray.insertAt(1, first)

In development, this works as expected. But in a production build, this code results in the first item being duplicated (as if the removeAt did not happen).

There are already some failing tests for this issue, as long as we run the tests in production mode:

not ok 19 Chrome 124.0 - [23 ms] - integration - Dependent State: reordering a fragment array dirties the fragment array and owner record
    ---
        actual: >
            3
        expected: >
            2
        stack: >
                at Object.<anonymous> (http://localhost:7357/assets/tests-28c38f66d4715374e954e49f4d4e8a30.js:39:22)
        message: >
            fragment array length is maintained
        negative: >
            false
        browser log: |
    ...

not ok 110 Chrome 124.0 - [23 ms] - unit - `MF.fragmentArray`: changes to array contents change the fragment array `hasDirtyAttributes` property
    ---
        actual: >
            false
        expected: >
            true
        stack: >
                at Object.<anonymous> (http://localhost:7357/assets/tests-28c38f66d4715374e954e49f4d4e8a30.js:306:193)
        message: >
            fragment array is returned to clean state
        negative: >
            false
        browser log: |
    ...

Some of the existing tests depend on Ember.assert throwing an exception. To make this work in production mode, I've replaced assert.throws with expectAssertion, which is similar to ember-qunit-assert-helpers and to ember's own expectAssertion helper.

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
@dwickern dwickern marked this pull request as ready for review May 3, 2024 23:41
@knownasilya knownasilya added the bug Used when the PR fixes a bug included in a previous release. label May 6, 2024
@knownasilya knownasilya merged commit f04e04a into adopted-ember-addons:master May 6, 2024
7 of 11 checks passed
@knownasilya
Copy link
Collaborator

Released as v6.0.9

@dwickern dwickern deleted the fix-array-update branch May 6, 2024 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used when the PR fixes a bug included in a previous release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants