updateComponent hook bug with {{each}} helpers in v3.18

[sandydoo]

Live reproduction of both versions: https://sandydoo.github.io/ember-test-update-hook-rendered/
Reproduction repo: https://github.com/sandydoo/ember-test-update-hook/

TL;DR In v3.18, the updateComponent hook is called on every nested component in an {{each}} and {{each-in}} helper when a single element or key/value pair is modified.

I've been testing out a custom component manager with the updateHook enabled.

capabilities = capabilities('3.13', {
    updateHook: true // <- Hook enabled
});

The updateComponent hook behaves strangely with {{each}} and {{each-in}} helpers.

If I render a custom component for every element in an array and append/remove an element, then the updateComponent hook is called on every component in the array.

If I render a TrackedMap and modify a single key/value pair, then the hook is called on every single component except for the one actually modified.

Here are the test examples:

Modify a value in a TrackedMap

v3.16: the updateComponent hook is called on the one component whose key/value pair was modified. 👍
v3.18: the updateComponent hook is called on every single key/value pair in the TrackedMap except for the one modified. 👎

<button class="button" {{on 'click' this.updateMap}}>Update</button>

{{#each-in this.testMap as |key value|}}
  {{!-- Just displays the key and value --}}
  <Test @key={{key}} @value={{value}} />
{{/each-in}}

Append to an Ember array

v3.16: the updateComponent hook is not called. 👍
v3.18: the updateComponent hook is called for every single existing component. 👎

<button class="button" {{on 'click' this.updateArray}}>Update</button>

{{#each this.testArray as |value key|}}
  <Test @key={{key}} @value={{value}} />
{{/each}}

Links to a reproduction of the issue are at the top.
Not yet sure if this is specific to custom component managers or a more general issue.

[rwjblue]

This is likely another result from the rendering engine update in 3.17, the entire iterable system was revamped (we’ve already fixed a few bugs in this space in 3.18.1).

@sandydoo - Do you think you could try to put together a failing test case here (or in glimmer-vm)? It would probably make it a bit easier for us to dig in...

@pzuraq - would you mind reviewing? Maybe the specific symptoms might jump out at you and point to the culprit...

[sandydoo]

@rwjblue I've added two failing test cases to the component manager test suite: modifying a key/value pair in an object and pushing an item to an array. They pass on v3.16.8 and fail on v3.18.1.

Update: and a glimmer-vm test sandydoo/glimmer-vm@fbd7810. I don't think this test makes sense though. Since the whole array has changed, it makes sense that the iterables are updated...

[sandydoo]

Update

I can replicate this with render modifiers as well. I guess that's not too surprising.

v3.16.8: The {{did-update}} modifier is not triggered when pushing to the array.
v3.18.1: The {{did-update}} modifier is triggered for every existing rendered value when pushing to the array.

<button {{on 'click' this.updateArray}}>Update</button

{{#each this.testArray as |value index|}}
  <div {{did-update this.didUpdateElement index}}>{{value}}</div>
{{/each}}

testArray = A();

@action
updateArray() {
  this.testArray.pushObject('test');
}

@action
didUpdateElement(_element, index) {
  console.log(`Updating ${index}`);
}

[rwjblue]

Awesome, thank you for those test cases!

[sandydoo]

Btw, I was digging around in glimmer-vm and something caught my eye. Now I could be totally off-track here, but is it possible that the check for whether the iterable item has actually changed was misplaced in the move to the new iterable system?

It seems like, either here or somewhere further upstream(here and here), we're forcefully marking the tag as dirty. Then, as the VM is going through its list of opcodes, it ends up calling the update component opcode, thinking that something has changed. 🤷 I can't seem to find a point in the loop where we're saying "hey, this tag/value hasn't changed, so let's move on".

https://github.com/glimmerjs/glimmer-vm/blob/e8e2fc6f39a60baac2b72c1a19aea9585b162c47/packages/%40glimmer/reference/lib/template.ts#L310

v3.18.1

--------------------- Ember ---------------------

1. Push to Ember array
2. Call notifyPropertyChange
3. Call markObjectAsDirty
4. Call dirtyTagFor on the property (index) changed, '[]', and the array itself

--------------------- Glimmer ---------------------

For existing items:

5. Retain iterable. Call update on IterationItemReference.
6. Dirty tag and set itemValue <- That's it – tag is now dirty even though value hasn't changed.

update(value: T) {
  dirtyTag(this.tag);
  this.itemValue = value;
}

v3.16.8

--------------------- Ember ---------------------

1. Push to Ember array
2. Call notifyPropertyChange
3. Call markObjectAsDirty with key '[]'
4. Call dirty on propertyTag for '[]'

--------------------- Glimmer ---------------------

5. Call update on EachIterable

--------------------- Ember ---------------------

6. Call update on UpdatableReference
7. Check if (value !== _value) before marking as dirty and updating internal value
   

   ember.js/packages/@ember/-internals/glimmer/lib/utils/references.ts

   Lines 263 to 271 in 4810c54

   	update(value: Opaque): void {
   	let { _value } = this;
   	if (value !== _value) {
   	dirty(this.tag);
   	this._value = value;
   	}
   	}
   	}

Removing this equality check in v3.16.8 causes similar behaviour seen in v3.18.1

[sandydoo]

It seems like this is a general regression affecting iterables. All iterable items are updated, regardless of whether they have been modified.

Here's a simple test:

{{#each this.list as |item|}}
  {{item}}
{{/each}}

this.list = A(['one', 'two']);
// ...
this.list.pushObject('three');

I've printed out the opcodes into the console. Notice the lastRevision.

v3.16.8 Existing items aren't marked as dirty. Revision is not bumped.

v3.18.1 Existing items are marked as dirty. All items now at revision 6.

https://github.com/sandydoo/glimmer-vm/tree/iterable-bug

[sandydoo]

So the following works sandydoo/glimmer-vm@4a3613f, but it will break a bunch of Glimmer tests. These were ported from Ember's test suite and obviously no longer use Ember arrays or set to update values. As a result, any test that modifies a value in an array of objects fails; it is not longer forcefully updated. Here's an example of what I mean.

Anyways, not much else I can do. Are these iterable items intentionally updated, but are now triggering side-effects (like did-update callbacks) back in Ember? Or are we just missing an equality check? Need a review from someone working on the vm.

Any thoughts, @rwjblue & @pzuraq?

[pzuraq]

@sandydoo sorry just getting to this today. I think you are correct there, and I can't currently remember why that changed during the update - looking back, Iterables definitely used to check the value of the reference before dirtying it.

The tests in the VM I think are wrong here, part of the mismatch between Ember.js and the test internals. You're correct, they don't have a set function or another way of tracking. I'm going to add a @tracked decorator to the test helpers, because we should be unifying the test behavior so it reflects the actual embedding environment moreso.

Thanks for all the work here, it would have taken me a while to follow this all the way down! This has been a huge help 😄

[sandydoo]

@pzuraq Awesome! I'm glad this was useful 😄