Destroyables

[pzuraq]

Rendered

[rwjblue]

This is really great, thank you for working on it!

I think we may need a couple additional things to allow for introspection:

• isDestroying(obj)
• isDestroyed(obj)

IMHO, these are somewhat important so that we can "mark" all of the graph that will be destroyed before calling all of the registered destructors and that the object itself can determine if it is in a destroying state or not before doing any work.

[rwjblue]

@pzuraq - Another feature that ember-lifeline has, that is pretty nice (and we would either have to drop there or not be able to migrate to this builtin API) is that we can assert that all destroyables that were ever registered have been ran. This is used in testing infrastructure (see the documentation here) to ensure that folks don't accidentally forget to properly destroy(obj) something that they should have (the assumption here is that at the end of the test, all destroyables that were created during the test are ran).

[rwjblue]

Thanks for the updates here @pzuraq, looks like great progress!

[rwjblue]

Does it also mark each of the children?

[pzuraq]

It does while calling destroy() on the children, since it recurses. It does not mark them as destroying before running its own destructors.

[rwjblue]

After discussing this in the meeting today, we are very excited to move this RFC into final comment period!

[ming-codes]

Given how @pzuraq loves decorators, I’m surprised to see there are no mention of decorators in this RFC 😏

[BryanCrotaz]

Suggestion. Create a codemod to convert willDestroy to using destroyables instead

[buschtoens]

If calling unregisterDestructor with a non-registered destructor throws an error, and there is no function to check whether a destructor is associated with a destroyable, it means that users need to implicitly or explicitly bookkeep what destructors were registered with what destroyables.

I like that it throws an error, but I would recommend to expose another function like isDestructorRegistered(obj, destructor) / hasDestructor(obj, destructor).

[buschtoens]

Will isDestroying / isDestroyed still return true though?

[pzuraq]

Yes, it will.

[buschtoens]

I overlooked the keyword Schedule, meaning that the action happens asynchronously / next tick / etc. and thought that this algorithm actually results in marking the parent, calling the parent destructors already and only then marking the children and calling the children's destructors.

Just adding this here, in case I am not the only one.

Do we wanna make any guarantees with regards to the timing of scheduling? Is it scheduled onto a specific runloop queue (destroy?), next tick, or no guarantees on that at all?

[pzuraq]

We do not want to make guarantees about timing, as timing could change per-environment, or could change in the future. The only guarantee is relative timing, as outlined in this RFC.

[buschtoens]

Suggested change

	- Calling `unregisterDestructor` on a destroyed object should throw an error.
	- Calling `unregisterDestructor` on a destroyed object or object that is being destroyed should throw an error.

Is it legal to unregister a destructor from an object while it is being destroyed (isDestroying)?

[buschtoens]

It would be helpful to know which objects did not finish destroying in this case. The objets could be exposed as a property on the error.

[buschtoens]

In my naive polyfill implementation, this function leaks state / failures between test runs, as there is no way to reset the hidden module state: ember-polyfills/ember-destroyable-polyfill#2

test('should fail', function () {
  const obj = {};
  registerDestructor(obj, () => {});
  // run(() => destroy(obj));
  assertDestroyablesDestroyed();
});

test('should pass, but fails', function () {
  const obj = {};
  registerDestructor(obj, () => {});
  run(() => destroy(obj));
  assertDestroyablesDestroyed();
});

How are ember-qunit / ember-mocha supposed to reset the module state in @ember/destroyable after each test run? require.unsee('@ember/destroyable')? Or is this actually a design flaw?

[pzuraq]

assertDestroyablesDestroyed could probably do the cleanup during test builds. We would have to track some extra state in DEBUG builds, use a normal Map instead of a WeakMap, but that's something we've done before.

[rwjblue]

I've commented on the issue linked above (in the polyfill) to explain how this should work. I don't think this is a gap in the public APIs proposed (but the assertion would leverage internal private APIs).

[buschtoens]

I made a polyfill for this RFC: ember-destroyable-polyfill

It's not perfect yet: The test suite could be expanded and the integration with CoreObject / the hooks-based system could be improved, but AFAICT it works.

[MelSumner]

Thank you for updating the learning content! 👍

[rwjblue]

Thanks for working through this with us!

[sandstrom]

@buschtoens Awesome addon! Just installed it, works great! 💯