prevent mixin from replacing an existing property

[pixelzoom]

extend is used throughout PhET code to handle mixins. One of the pitfalls with mixins is that you might overwrite a property that already exists. extend currently doesn't do any checking for this. Please add an assertion that fails when you attempt to mix in a property that already exists.

[pixelzoom]

I tried adding this assertion to extend:

assert && assert( !obj.hasOwnProperty( prop ), 'property already exists: ' + prop );

But then discovered that extend is also used by inherit. So this assertion fails whenever a function is overridden by a subtype. Highly recommended to use different logic for mixin vs inheritance.

[jonathanolson]

extend is used throughout PhET code to handle mixins. One of the pitfalls with mixins is that you might overwrite a property that already exists. extend currently doesn't do any checking for this. Please add an assertion that fails when you attempt to mix in a property that already exists.

It's also meant to be a mostly "drop-in" replacement for _.extend that also properly handles ES5 setters/getters.

If we're adding this assertion to extend, presumably that would also disallow our pattern of default options:

var options = { a: 5 };

// Would error out because we are overriding 'a'
options = extend( {
  a: 3 // default value
}, options );

Presumably we'd want to create a new (similar) function (or allow options somehow?) with the assertion for cases where we don't want to allow overriding.

Any preferences on a name for this function? extendOnly? augment?

[pixelzoom]

@jonathanolson said:

If we're adding this assertion to extend, presumably that would also disallow our pattern of default options

I didn't realize that extend.js was being used to extend options, I always used _.extend. Searching for "options = extend" it appears to be used only in Poolable. Why is _.extend not appropriate in Poolable?

Any preferences on a name for this function? extendOnly? augment?

I guess it depends on how (or whether) we address phetsims/scenery#700. But...
mixin for Mixins?
compose for Traits? With support for verifying that traits are composed with types that implement methods required by a trait?

[jonathanolson]

I didn't realize that extend.js was being used to extend options, I always used _.extend. Searching for "options = extend" it appears to be used only in Poolable.

_.extend mucks up ES5 getters/setters, since it ignores setters, and uses whatever value for the getter exists at the time it's called (it actually calls the getter to retrieve the value to override on the object).

extend (phet-core's version) copies over setters/getters nicely.

For example:

var es5 = {
  set foo( value ) { this._foo = 'ES5 setter: ' + value; },
  get foo() { return this._foo + ' (ES5 getter)'; }
};

// Clobbers ES5 setter/getter
var lodashed = _.extend( {}, es5 );
lodashed.foo = 'test';
lodashed.foo; // returns "test"

// Copies over ES5 setter/getter
var phetcored = phetCore.extend( {}, es5 );
phetcored.foo = 'test';
phetcored.foo; // returns "ES5 setter: test (ES5 getter)"

Why is _.extend not appropriate in Poolable?

It would technically work in this case, but would fail out if we add getters/setters. I generally prefer phet-core's extend whenever it would have general prototypes (where we might have getters/setters), and use lodash's _.extend whenever things are just data objects.

I guess it depends on how (or whether) we address phetsims/scenery#700. But...
mixin for Mixins?
compose for Traits? With support for verifying that traits are composed with types that implement methods required by a trait?

Presumably this would be a function called by both mixins and traits (since they'd both want to prevent silently overriding things). Traits would have additional type-checks for the types in their call sites.

[pixelzoom]

@jonathanolson

_.extend mucks up ES5 getters/setters ...

When would we have ES5 getters/setters in an options hash? Just trying to understand why this one usage in Poolable (line 46).

Presumably this would be a function called by both mixins and traits (since they'd both want to prevent silently overriding things). Traits would have additional type-checks for the types in their call sites.

Sounds right to me.

[jonathanolson]

When would we have ES5 getters/setters in an options hash? Just trying to understand why this one usage in Poolable (line 46).

Bah, didn't realize the extend was called on an options object. Yes, should be safe for _.extend in that case. I'll make the change.

Any thoughts on the preferred name?

Any preferences on a name for this function? extendOnly? augment?

[pixelzoom]

Any thoughts on the preferred name?

In Scala, both mixins and traits are "mixed in". So... mixIn? Or do you want a name that is more general, not applicable just to mixins and traits?

[jonathanolson]

In Scala, both mixins and traits are "mixed in". So... mixIn? Or do you want a name that is more general, not applicable just to mixins and traits?

Was thinking more general, since we may want to extend (disallowing overriding) in other cases.

[pixelzoom]

lodash uses "overwrite" instead of "override". (Override typically refers to methods in OO programming). extendNoOverwrite ? extendOnly? (as you suggested)