Ember.js: Modifier Managers should not re-consume their args on destroy

[NullVoxPopuli]

According to the current modifier manager spec:

destroyModifier will be called when the modifier is no longer needed. This is intended for performing object-model level cleanup.

then, in the following code example:

 destroyModifier(instance, args) {
    if (instance.willDestroyDOM !== undefined) {
      instance.willDestroyDOM();
    }
  }

args are passed to destroyModifier.

I think this is a bug because:

• it means that args are all re-consumed on _destruction_ of a render / component tree
• if guards in templates no longer can guarantee the safety of the shape of data
• in order for modifiers' destruction to not trigger exceptions wherever args' derived data is derived, you need to add additional guards all over your code, or do foo?.bar?.bax everywhere (this generates a lot of boilerplate)

---

I propose we _remove_ args from the destroyModifier hook so that

• none of the above issues exist anymore
• teardown is slightly faster due to not re-consuming args

---

For example, today, the following isn't safe:

{{#if this.data.foo}}
  <SomeComponent @data={{this.data}} />
{{/if}}

// some-component
export default setComponentTemplate(hbs`
  <span {{some-modifier this.nestedData}}>breaks</span>
`, class extends Component {
  get nestedData() {
    // you should be able to assume foo exists, because of the condition in the previous template
    // but today, you can't assume that :(
    return this.args.data.foo.dataOnFoo;
  }
});

Minimal Reproduction / Description of the Problem: https://ember-twiddle.com/5a3fa797b26e8807869340792219a5ee?openFiles=components.demo%5C.hbs%2Ccomponents.demo%5C.hbs

Relevant Discord Threads:

• one
• two

[NullVoxPopuli]

re: discord thread: https://discordapp.com/channels/480462759797063690/608346628163633192/743822662681493504

@pzuraq says that wrapping in a proxy so that args are only consumed when used has been a potential solution.

that would solve this! :D
(and would be a great way to not remove functionality some folks may be relying on)

[mattmcmanus]

Thanks for writing this up @NullVoxPopuli. This problem is currently biting us at Yapp and had broken my brain for the last couple days.

[mydea]

I agree that the current behavior, at the very least, _feels_ like a bug, and the resulting problems are very hard to pinpoint, and unintuitive. So any solution to this would be greatly appreciated! Thanks for writing this up!

[rwjblue]

IMHO, this is a bug. It's basically the reason that we use an args proxy for component managers, and a recent PR by @chancancode made that a bit more generic so that we could use it for our modifier managers.

I'm going to move this into an issue on the emberjs/ember.js repo, so we can work on a fix (instead of an RFCs issue).

[rwjblue]

The path forward here is to make this code sharable between the various manager types (modifier and component now, but soon helpers):

https://github.com/emberjs/ember.js/blob/984e69c2be41acc243805aae339cb7499fa2d9cc/packages/%40ember/-internals/glimmer/lib/component-managers/custom.ts#L159-L238

• Extract ^ to a utility (it should be "primed and ready" I think)
• Update custom modifier manager to do something more like this:

https://github.com/emberjs/ember.js/blob/984e69c2be41acc243805aae339cb7499fa2d9cc/packages/%40ember/-internals/glimmer/lib/component-managers/custom.ts#L284-L289

Instead of always reifying:

https://github.com/emberjs/ember.js/blob/984e69c2be41acc243805aae339cb7499fa2d9cc/packages/%40ember/-internals/glimmer/lib/modifiers/custom.ts#L149

https://github.com/emberjs/ember.js/blob/984e69c2be41acc243805aae339cb7499fa2d9cc/packages/%40ember/-internals/glimmer/lib/modifiers/custom.ts#L161

https://github.com/emberjs/ember.js/blob/984e69c2be41acc243805aae339cb7499fa2d9cc/packages/%40ember/-internals/glimmer/lib/modifiers/custom.ts#L66

[rwjblue]

I've pushed up a quick stab of this over in https://github.com/emberjs/ember.js/pull/19163