[BUGFIX lts] Ensure destroy methods on CoreObject are invoked.
#19106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FWIW, I am working on another PR (that won't be |
rwjblue
force-pushed
the
core-object-with-destroy
branch
4 times, most recently
from
12fe448
to
5b1c358
Compare
rwjblue
commented
Aug 25, 2020
| @@ -256,6 +266,7 @@ class CoreObject { | |||
| }); | |||
| } | |||
|
|
|||
| registerDestructor(self, ensureDestroyCalled, true); | |||
There was a problem hiding this comment.
Note that this is specifically an eager destructor to match the previous semantics. The .destroy method would always have gotten called before the downstream objects are marked as isDestroying and before the instances own .willDestroy is called.
When an instance of `CoreObject` is destroyed with `@ember/destroyable` (or the polyfill when using Ember 3.20+), the instances own `destroy` method is never called. In _general_ "this is fine" because the majority of consumers are using `willDestroy` for their custom object teardown needs. Unfortunately, this isn't _quite_ good enough and any consumers that are directly relying on extending from `CoreObject` and having a custom `destroy` method called will no longer function. The prime example of this is the `Owner` instance itself. Currently, running `destroy(owner)` **does not** actually destroy any instantiated objects in the container. This is because `ContainerProxyMixin` implements `destroy` in order to call `this.__container__.destroy()` which is where the actual cleanup happens. That whole system should be refactored to leveraging `@ember/destroyable` system directly, but in the meantime this case (and any others where folks have implemented a destroy method in Mixins or other subclasses of Ember.CoreObject) should **absolutely** be fixed. This fix ensures that any `destroy(someInstance)` calls on a `CoreObject` directly invoke the `destroy` method on the instance in the same manner as Ember < 3.20 (`.destroy()` is called eagerly, before `willDestroy`).
rwjblue
force-pushed
the
core-object-with-destroy
branch
from
5b1c358
to
4d55dae
Compare
krisselden
approved these changes
Aug 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an instance of
CoreObjectis destroyed with@ember/destroyable(or the polyfill when using Ember 3.20+), the instances owndestroymethod is never called. In general "this is fine" because the majority of consumers are usingwillDestroyfor their custom object teardown needs. Unfortunately, this isn't quite good enough and any consumers that are directly relying on extending fromCoreObjectand having a customdestroymethod called will no longer function.The prime example of this is the
Ownerinstance itself. Currently, runningdestroy(owner)does not actually destroy any instantiated objects in the container. This is becauseContainerProxyMixinimplementsdestroyin order to callthis.__container__.destroy()which is where the actual cleanup happens. That whole system should be refactored to leveraging@ember/destroyablesystem directly, but in the meantime this case (and any others where folks have implemented a destroy method in Mixins or other subclasses of Ember.CoreObject) should absolutely be fixed.This fix ensures that any
destroy(someInstance)calls on aCoreObjectdirectly invoke thedestroymethod on the instance in the same manner as Ember < 3.20 (.destroy()is called eagerly, beforewillDestroy).