phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Traits should use `assertHasProperties` #85

Closed zepumph closed 4 years ago

zepumph commented 4 years ago

Over in EnabledComponent, we created and used PHET_CORE/assertHasProperties.js to declare the dependencies a trait has on the core type it is being mixed into. It was nice and valuable, and we should use this else where.

Tagging parent issue of #54.

Here is a list of the usages of @trait that doesn't currently use assertHasProperties:

Most are for Nod, and most are for a11y types. So assigning to a11y devs.

zepumph commented 4 years ago

I added these to the a11y types listed above. I see that it isn't needed for Leaf.js so I checked it off.

I wanted to add this patch to Paintable but found the Paintable is often initialized before the Node supertype is.

Index: js/nodes/Paintable.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/nodes/Paintable.js   (revision efb89d952bc66f5c76c0728fa7534cad7605f7fe)
+++ js/nodes/Paintable.js   (date 1590618746381)
@@ -9,6 +9,7 @@
 import Property from '../../../axon/js/Property.js';
 import LineStyles from '../../../kite/js/util/LineStyles.js';
 import arrayRemove from '../../../phet-core/js/arrayRemove.js';
+import assertHasProperties from '../../../phet-core/js/assertHasProperties.js';
 import extend from '../../../phet-core/js/extend.js';
 import inheritance from '../../../phet-core/js/inheritance.js';
 import platform from '../../../phet-core/js/platform.js';
@@ -90,6 +91,9 @@
        * @protected
        */
       initializePaintable: function() {
+
+        assertHasProperties( this, [ '_drawables' ] );
+
         this._fill = DEFAULT_OPTIONS.fill;
         this._fillPickable = DEFAULT_OPTIONS.fillPickable;

I see this in Path and Text. As a result the above isn't true. @jonathanolson this feels like a code smell to me. Why can Paintable, which supposedly depends on certain properties of Node (as it is a trait), get initialized before the super call to Node in these cases? Can we swap that so that the above patch can be applied?

jonathanolson commented 4 years ago

Applied the swap above, that "seems" better in general. Let me know if you need anything else.

zepumph commented 4 years ago

Excellent! Thank you.