Should destroyObject also replace .destroy()?

[jjspace]

While reviewing #12129 I finally noticed the if !isDestoryed() > .destroy() pattern and it felt very odd to me.

if (!this._environmentMapManager.isDestroyed()) {
  this._environmentMapManager.destroy();
}

My comment:

It feels like the responsibility for this check should live in the destroy function itself. I feel like you should be able to just call destroy() regardless and let it handle checking if it's already destroyed and essentially turn into a noop function. Then in this function the logic would just be "We know we want to guarantee this is destroyed so call destroy()" and no wrapper check for if it's already destroyed?

Looking at the destroyObject function I see we delete all the properties and replace the .isDestroyed function with a function that only returns true.

cesium/packages/engine/Source/Core/destroyObject.js

Line 53 in 192ab06

object.isDestroyed = returnTrue;

I think a potentially easy path forward to streamline this pattern and fit the way I expected it to work would be to also (or maybe instead of) replace the .destroy() function with a noop () => {}.

This would allow us to rewrite that original example as just one line and not have to care about checking first. The responsibility is with the original object even though it's all been deleted by destoryObject

this._environmentMapManager.destroy();