Guard against shadowing for mixins and traits

[jessegreenberg]

This came out of discussion in phetsims/phet-info#92 and phetsims/phet-info#88. When using mixin/trait pattern there is no guard against accidentally overriding functions in the mixin/trait.

It was recommended that someone examine our mixin/trait pattern and recommend a solution for this. Adding to developer meeting to discuss with @ariel-phet priority and who should investigate.

[jessegreenberg]

I also found #29, which is very related. Though I think discussion on the 2/11/19 sofware design patterns meeting was more concerned with accidental function overriding than accidentally replacing properties.

[samreid]

Accidental shadowing can also occur for class extends and inherit. Perhaps we can find a solution that helps us address these cases in addition to mixins/traits. Do we ever have intentional shadowing?

[Denz1994]

Dev Meeting 02/14/19

AP: Return to dev meeting with a recommendation to handle accidental overrides.

SR: Consider using ES6 proxy to catch shadowing. May not be compatible with all browsers.

JO: A better pattern for checking for existence before the mixin may avoid proxy usage.

[jessegreenberg]

Discussed during core hours 2/25/19, we would like to guard against shadowing for methods and fields when using the mixin/trait pattern. If the solution to this will work with inherit that is great, but since we are generally writing new code with extends now, it isn't worth spending much time on that.

[jessegreenberg]

Part of this might include creating a more concrete trait file that defines our pattern for this. Right now the pattern for using mixin/trait involves re-writing the code each time for modifying the prototype and everything else.

[pixelzoom]

This came up again in the context of phetsims/sun#257. We are using mixin/trait heavily for scenery and a11y. Now we're adding it to UI components in sun and scenery-phet. We need to guard against accidentally shadowing properties (lowercase 'p') of the host class.

Something as simple as this might do the job:

function assertVerifyMixinProperties( object, properties ) {
   properties.forEach( property => 
    assert( !object.hasOwnProperty( property ), `mixin will shadow host property: ${property}` )
}

// In sun.EnabledComponent
assert && assertPropertyOverrides( this, [ 'enabledProperty', 'setEnabled', 'getEnabled', 'disposeEnabledComponent', '_disposeEnabledComponent'  ] );

Labeling for developer meeting.

[samreid]

I think @zepumph pointed out that hasOwnProperty is not sufficient to determine the existence of properties within the prototype chain.

[pixelzoom]

Right, hasOwnProperty is not sufficient. We need to look all the way up the class hierarchy. Or I should say "up the prototype chain", since JavaScript really doesn't have classes.

[zepumph]

assertHasOwnProperties.js was just created over in phetsims/sun#257 and holds the logic that will likely be needed here for a general solution to test that items don't yet exist. Perhaps we can turn that into a function that returns a boolean so that we can use it in either direction.

[zepumph]

• What ever is decided here should be implemented in EnabledComponent immediately to make sure that this doesn't yet have an enabledProperty.

[pixelzoom]

Perhaps we can turn that into a function that returns a boolean so that we can use it in either direction.

I considered that when I suggested assertHasOwnProperties over in phetsims/sun#257, and then proposed assertVerifyMixinProperties in #54 (comment). But I think we will want an assertion failure that identifies a specific property, not just "some property already exists in host class".

[jonathanolson]

So we're looking for a library function that scans a specific object's prototype to determine if it has a definition? Then ideally a helper function that will scan a list of property names and error out nicely if they exist?

I'm happy to write that and provide it, let me know.

[zepumph]

@jonathanolson I think part of the appeal is to check non prototype elements to. For example making sure that this.enabledProperty isn't defined before EnabledComponent.initializeEnabledComponentsets it. I can't really think of a way to implement that in a way that isn't manual in eachinitialize*` method.

Are we interested in doing this for the prototype methods too? Perhaps this could be a replacement for the extend function in mixins. And part of the prototype extension is to make sure that it isn't blowing anything away.

@jonathanolson I would say go for it and we can take it for a test drive.

[jessegreenberg]

Visited this issue 5/21/20 developer meeting and still think this would be useful, particularly since a11y related code is leveraging trait/mixin pattern more.

[samreid]

I'd like to understand how this problem would appear when using the class-factory pattern identified in phetsims/phet-info#143

[jonathanolson]

This would be similar to the problem of "how do you know if you're overwriting a method/property with normal inheritance", which we currently don't auto-detect.

[jonathanolson]

In dev meeting, we're going to put this on hold until phetsims/phet-info#143 is completed.

[samreid]

TypeScript now catches shadowing and will error unless you annotate as override. Closing.