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 glimmer render error when creating fragment arrays #466

Merged
merged 5 commits into from
May 30, 2023

Conversation

dwickern
Copy link
Contributor

@dwickern dwickern commented May 26, 2023

Fixes #357

Constructing a record throws an error:

store.createRecord('person', {
  titles: ['Hand of the King', 'Master of Coin'],
});
Error: Assertion Failed: You attempted to update `[]` on `<(unknown):ember176:owner(null)>`, but it had already been used previously in the same computation.  Attempting to update a value after using it in a computation can cause logical errors, infinite revalidation bugs, and performance issues, and is not supported.

`[]` was first used:

- While rendering:
  -top-level
    application
      index
        PersonComponent

Stack trace for the update:
    at dirtyTagFor (http://localhost:4201/assets/vendor.js:49934:37)
    at markObjectAsDirty (http://localhost:4201/assets/vendor.js:9751:32)
    at notifyPropertyChange (http://localhost:4201/assets/vendor.js:9794:5)
    at Class.notifyPropertyChange (http://localhost:4201/assets/vendor.js:22511:39)
    at Class.notify (http://localhost:4201/assets/vendor.js:85031:14)
    at Class.replace (http://localhost:4201/assets/vendor.js:85059:12)
    at Class.setObjects (http://localhost:4201/assets/vendor.js:21903:12)

This happens when we initialize a fragment array while a render loop is running.

@dwickern
Copy link
Contributor Author

dwickern commented May 26, 2023

It's a problem with our property change tracking, but I'm not sure how to fix it.

notify() {
this._isDirty = true;
if (HAS_ARRAY_OBSERVERS && this.hasArrayObservers && !this._hasNotified) {
this.retrieveLatest();
} else {
this._hasNotified = true;
this.notifyPropertyChange('[]');
this.notifyPropertyChange('firstObject');
this.notifyPropertyChange('lastObject');
}
},
get length() {
if (this._isDirty) {
this.retrieveLatest();
}
// By using `get()`, the tracking system knows to pay attention to changes that occur.
// eslint-disable-next-line ember/no-get
get(this, '[]');
return this._length;
},

createRecord('person', { titles: ... }) desugars to:

const person = store.createRecord('person');
person.titles.setObjects(['Hand of the King', 'Master of Coin']);

MutableArray.setObjects calls length and then replace, which effectively calls get('[]') followed by notifyPropertyChange('[]')

@dwickern
Copy link
Contributor Author

dwickern commented May 26, 2023

I've added a second test which depends on the get(this, '[]') call inside length to initiate auto-tracking. If I remove that line, the first test passes but the second test fails. The code was originally from emberjs/data#7330.

My goal here is to get both tests passing.

I should add that this bug was not introduced in model-fragments v6, it's also present in v5.

@dwickern
Copy link
Contributor Author

My workaround 10a7825 implements setObjects without calling length. Both tests pass. It's not a perfect solution, but probably good enough?

@dwickern
Copy link
Contributor Author

This issue was reported before in #357

@dwickern dwickern marked this pull request as ready for review May 29, 2023 22:05
@dwickern dwickern changed the title failing test for glimmer render error Fix glimmer render error when creating fragment arrays May 29, 2023
@knownasilya knownasilya added the bug Used when the PR fixes a bug included in a previous release. label May 30, 2023
Copy link
Collaborator

@knownasilya knownasilya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad you found a workaround

@knownasilya knownasilya merged commit f4f4daf into adopted-ember-addons:master May 30, 2023
@knownasilya
Copy link
Collaborator

Released as 6.0.1

@dwickern dwickern deleted the render-error branch May 30, 2023 14:35
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.

[Bug Ember 3.15.0+] Creating a tracked model with a fragment array throws back-tracking assertions
2 participants