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

[patocallaghan]

I'm currently trying to upgrade our app from ember-source 3.14.x to 3.15.x and I've found a blocker which is preventing our upgrade.

From ember-source 3.15.0-beta.4 it appears that tracked models which have properties using fragmentArray will throw a backtracking rerender assertion causing things to blow up.

There are two suspect commits in ember-source where this was introduced.

emberjs/ember.js#17834 [BUGFIX] Prevents autotracking ArrayProxy creation
emberjs/ember.js#18554 [BREAKING BUGFIX] Adds autotracking transaction

I've created an extremely simplified test case in #358 and it throws the following assertion:

Error: Assertion Failed: You attempted to update `[]` on `Array`, 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:
  
  application
    index
      my-component
        this.record

Stack trace for the first usage: 
    at get (http://localhost:7357/assets/vendor.js:14851:32)
    at Class._addArrangedContentArrayObserver (http://localhost:7357/assets/vendor.js:28971:44)
    at Class.init (http://localhost:7357/assets/vendor.js:28831:12)
    at Class.init (http://localhost:7357/assets/vendor.js:87960:12)
    at Class.superWrapper [as init] (http://localhost:7357/assets/vendor.js:30845:22)
    at initialize (http://localhost:7357/assets/vendor.js:29137:9)
    at Function.create (http://localhost:7357/assets/vendor.js:29720:9)
    at createFragmentArray (http://localhost:7357/assets/vendor.js:88297:32)

Stack trace for the update:
    at dirtyTagFor (http://localhost:7357/assets/vendor.js:55641:11)
    at markObjectAsDirty (http://localhost:7357/assets/vendor.js:14006:32)
    at notifyPropertyChange (http://localhost:7357/assets/vendor.js:14043:5)
    at arrayContentDidChange (http://localhost:7357/assets/vendor.js:14144:7)
    at replaceInNativeArray (http://localhost:7357/assets/vendor.js:14210:5)
    at Array.replace (http://localhost:7357/assets/vendor.js:27255:39)
    at Class.setupData (http://localhost:7357/assets/vendor.js:88000:15)

The problem appears to happen in the following lines, specifically 298 (where the array is created) and 300 (where the array is updated) https://github.com/lytics/ember-data-model-fragments/blob/c00e2b1a40b296e0c0c3c21ca0f463d12888cd27/addon/attributes.js#L298-L300

The array is first set up in createArray at

https://github.com/lytics/ember-data-model-fragments/blob/c00e2b1a40b296e0c0c3c21ca0f463d12888cd27/addon/attributes.js#L195-L208

the array is modified a second time in

https://github.com/lytics/ember-data-model-fragments/blob/f1c7060fb7560aaea26760e0270234823b9e20db/addon/array/stateful.js#L79

While in my test reproduction I am specifically marking the property as @tracked

@tracked selectedRecord = this.store.createRecord('my-model', { fragments: [] });

[patocallaghan]

This is also happening in the current ember-source 3.17.0-beta.4

[patocallaghan]

I created a corresponding issue in the ember.js repo too emberjs/ember.js#18773

[patocallaghan]

Just posting an update here. There was a bug on the Ember.js side and that was resolved but according to @pzuraq there are still some problems with Ember Data Model Fragment's implementation when setting up fragmentArrays. See the comment here emberjs/ember.js#18773 (comment)

So, this isn't intended, and we dug in and added it to the fix for #18770. Once that lands, it should prevent creating an ArrayProxy from entangling and causing this error.

However, I ran that branch against your test and it does still fail. This is because Model Fragments is also entangling the content array multiple places before mutating it, within its own code. Whenever you see get(this, 'content'), that will entangle the array, and then when you try to update it later it will throw a new error.

So, Model Fragments will likely still need to be refactored after we merge the fix for the Ember side of things.

I've followed up and I think the best approach here would be to refactor the code where fragmentArrays are setup, possibly have a separate codepath for initializing versus setup? I had an initial go at a refactor but it appears quite tricky, there seems to be quite a bit of historical usages and edge cases accounted for.

@jakesjews @slindberg @workmanw would anyone be up for a chat to talk about the right path forward here? I'd be happy to work on it.

[jakesjews]

@patocallaghan I've been sick the last few days but I can chat more after I'm better. I think it's ok to make some breaking changes in a new major version release if need be. I took over maintenance on this after the original author moved on so a lot of the historical usage is unknown to me as well.

[patocallaghan]

Sounds great @jakesjews. Let me know when you'd like to chat 👍

[knownasilya]

What's the status on this issue?

[patocallaghan]

@knownasilya myself and a colleague spiked a fix which resolved the initial test case I'd created but it didn't totally solve the issue as we got backtracking assertions in other parts of the code.

Due to the ongoing overhaul of the internals of model-fragments to use recordData and the rerenders coming from code which is currently being changed, it didn't feel like a good use of our time to try refactor further until that lands.

[GuillaumeCisco]

Has this issue been resolved?
I usually get this kind of error with ember fragment array too :/

index.js:128 Uncaught Error: Assertion Failed: You attempted to update `currentState` on `<client@model:field::ember2727:88cd118b-e152-11ea-919f-076ae1c59f30>`, 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.

`currentState` was first used:

- While rendering:
  ----------------
    this.field.hasDirtyAttributes

[GuillaumeCisco]

That's so weird it works, if I call a second time the fragment array...
Any news about that?

[GuillaumeCisco]

Ok, for the record, I just found out this error happens only if my model becomes dirty BEFORE I ever accessed (via computed or whatever) to the fragment array...

When the fragment array is created via setupData, propertyWasReset is called from the DirtyState which triggers a rolledBack triggering a internalModel.transitionTo('loaded.saved'); triggering this error...

Am I doing something wrong?

I'm working with ember 3.16 and ember-data-model-fragments 5.0.0-beta.0

[GuillaumeCisco]

What is really strange is that propertyWasReset defined as:

propertyWasReset(internalModel, name) {
      if (!internalModel.hasChangedAttributes()) {
        internalModel.send('rolledBack');
      }
    },

calls hasChangedAttributes which return false but in my own test, if I call changedAttributes here, I have changed attributes...
It looks like hasChangedAttributes is not correctly implemented in ember-data-model-fragments...

I replaced it by a dummy implementation like:

hasChangedAttributes() {
    return Object.keys(this.changedAttributes()).length;
  }

and now the error disappeared as rolledBack is no more sent...

What should we do?
@jakesjews @patocallaghan

[patocallaghan]

@GuillaumeCisco I wonder if you could create a failing test for the issue you are seeing? Looking at the open issues there are few which look to be related to changedAttributes.

[GuillaumeCisco]

Thank you @patocallaghan, I will do my best the moment I have enough time on that.
But I can clearly see there is an issue with the hasChangedAttributes...

[GuillaumeCisco]

Hey there,

I was not able to create a test with my specific use case, so I decided to update a very simple one dealing with hasChangedAttributes.
Here it is.

With its dummy fix here.

Let me know what you think about that.

[GuillaumeCisco]

Is this project dead?
I see that the last commit comes from 5 months ago.

Should I fork it if I want to use it?

[jakesjews]

@GuillaumeCisco we're currently moving ownership to ember-adopted-addons

[knownasilya]

Fixed in v6.0.1