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):

ember.js/packages/@ember/-internals/glimmer/lib/component-managers/custom.ts

Lines 159 to 238 in 984e69c

	let namedArgsProxyFor: (namedArgs: CapturedNamedArguments, debugName?: string) => Args['named'];
	if (HAS_NATIVE_PROXY) {
	namedArgsProxyFor = <NamedArgs extends CapturedNamedArguments>(
	namedArgs: NamedArgs,
	debugName?: string
	) => {
	let getTag = (key: keyof Args) => tagForNamedArg(namedArgs, key);
	let handler: ProxyHandler<{}> = {
	get(_target, prop) {
	let ref = namedArgs[prop as string];
	if (ref !== undefined) {
	return valueForRef(ref);
	} else if (prop === CUSTOM_TAG_FOR) {
	return getTag;
	}
	},
	has(_target, prop) {
	return namedArgs[prop as string] !== undefined;
	},
	ownKeys(_target) {
	return Object.keys(namedArgs);
	},
	getOwnPropertyDescriptor(_target, prop) {
	assert(
	'args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys()',
	namedArgs[prop as string] !== undefined
	);
	return {
	enumerable: true,
	configurable: true,
	};
	},
	};
	if (DEBUG) {
	handler.set = function(_target, prop) {
	assert(
	`You attempted to set ${debugName}#${String(
	prop
	)} on a components arguments. Component arguments are immutable and cannot be updated directly, they always represent the values that are passed to your component. If you want to set default values, you should use a getter instead`
	);
	return false;
	};
	}
	return new Proxy({}, handler);
	};
	} else {
	namedArgsProxyFor = <NamedArgs extends CapturedNamedArguments>(namedArgs: NamedArgs) => {
	let getTag = (key: keyof Args) => tagForNamedArg(namedArgs, key);
	let proxy = {};
	Object.defineProperty(proxy, CUSTOM_TAG_FOR, {
	configurable: false,
	enumerable: false,
	value: getTag,
	});
	Object.keys(namedArgs).forEach(name => {
	Object.defineProperty(proxy, name, {
	enumerable: true,
	configurable: true,
	get() {
	return valueForRef(namedArgs[name]);
	},
	});
	});
	return proxy;
	};
	}

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

ember.js/packages/@ember/-internals/glimmer/lib/component-managers/custom.ts

Lines 284 to 289 in 984e69c

	let namedArgsProxy = namedArgsProxyFor(named);
	let component = delegate.createComponent(definition.ComponentClass.class, {
	named: namedArgsProxy,
	positional: reifyPositional(positional),
	});

Instead of always reifying:

ember.js/packages/@ember/-internals/glimmer/lib/modifiers/custom.ts

Line 149 in 984e69c

let argsValue = reifyArgs(args);

ember.js/packages/@ember/-internals/glimmer/lib/modifiers/custom.ts

Line 161 in 984e69c

let argsValue = reifyArgs(args);

ember.js/packages/@ember/-internals/glimmer/lib/modifiers/custom.ts

Line 66 in 984e69c

registerDestructor(this, () => delegate.destroyModifier(modifier, reifyArgs(args)));

[rwjblue]

I've pushed up a quick stab of this over in #19163