{{will-destroy}} does not invoke rendering

[ch7sai]

> npm test
...
not ok 8 Chrome 78.0 - [66 ms] - Integration | Modifier | will-destroy: it should invoke UI re-rendering when changing tracked properties
---
actual: >
Element [data-dummy] exists once
expected: >
Element [data-dummy] does not exist
stack: >
at DOMAssertions.exists (http://localhost:7357/assets/test-support.js:9024:16)
at DOMAssertions.doesNotExist (http://localhost:7357/assets/test-support.js:9305:18)
at Object.<anonymous> (http://localhost:7357/assets/tests.js:172:34)
message: >
Element [data-dummy] does not exist
negative: >
false
Log: |
...

[hjdivad]

@pzuraq this test passes if the two blocks in the template are swapped.

// this failsthis.owner.register('template:components/test-component',hbs` {{#if this.showMessage}} <div data-dummy>Hello Ember</div> {{/if}} {{#if @shouldShow}} <div {{will-destroy (action "toggle")}}></div> {{/if}} `);// but this succeedsthis.owner.register('template:components/test-component',hbs` {{#if @shouldShow}} <div {{will-destroy (action "toggle")}}></div> {{/if}} {{#if this.showMessage}} <div data-dummy>Hello Ember</div> {{/if}} `);

The reason for this is that will-destroy modifiers are called mid-render, as opposed to did-insert and did-update modifiers which are called in env.commit. The difference is that if a modifier dirties a tag mid-render, the renderer will erroneously have that revision as its _lastRevision. This doesn't happen in the did-update case because by the time env.commit is called, _lastRevision has already been stashed.

This is easy enough to see in _renderRoots. will-destroy is called as part of root.render, instead of env.commit.

---

The rfc explicitly states that will-destroy might not be called in the same tick as DOM removal.

---

The obvious fix is to batch destroyables and invoke the modifiers in env.commit, but since this occurs after the element is already removed from the DOM it would make will-destroy into a did-destroy

1. What are your thoughts on adding a did-destroy element modifier with these semantics?

2. I'm very much of the opinion that dirtying during will-destroy should throw an error, since the current semantics are a landmine IMHO as they produce invalid revision state (ie the renderer will believe it has rendered revision X, but whether that's true or not depends entirely on whether DOM elements affected by the dirtied tag happen to occur later in the program than when the tag was dirtied). Your thoughts?

[pzuraq]

@hjdivad thanks for digging in and helping us root cause this!

1. I think it probably makes sense to do this, but we would likely need a new RFC to discuss it. There are some changes coming in the VM to destroyables in general that could force the issue.

2. That is very much an error. We had an assertion that prevented these errors, but in 3.13 that assertion regressed and allowed some failure cases to slip through. In 3.15 we patched the assertion and made it even stronger, so it actually should prevent this case from occurring at all. We should test in 3.15 to make sure of this, if not then that means the assertion may still have a few holes.