adopted-ember-addons / ember-data-model-fragments

Ember Data addon to support nested JSON documents
MIT License
370 stars 114 forks source link

Fix glimmer render error when creating fragment arrays #466

Closed dwickern closed 1 year ago

dwickern commented 1 year ago

Fixes https://github.com/adopted-ember-addons/ember-data-model-fragments/issues/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 commented 1 year ago

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

https://github.com/adopted-ember-addons/ember-data-model-fragments/blob/fcc8fc70b626ffba40eddcd50a6d0e93a2e19e29/addon/array/stateful.js#L61-L82

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 commented 1 year ago

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 https://github.com/emberjs/data/pull/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 commented 1 year ago

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

dwickern commented 1 year ago

This issue was reported before in #357

knownasilya commented 1 year ago

Released as 6.0.1