arturadib / agility

Javascript MVC for the "write less, do more" programmer
http://agilityjs.com
MIT License
542 stars 70 forks source link

Prototype model mutability bug #48

Closed tristanls closed 13 years ago

tristanls commented 13 years ago

There exists a configuration where a child agility object will modify the model of its prototype resulting in side effects.

If a child object extends a prototype that has an array or an object property in its model, and the child object does not specify its own corresponding property, the prototype array / object in the model will be returned to the child when child.model.get( property ) is called. This does not affect primitive properties like string.

The following test ( more included in the pull request ) exposes the bug:

var obj = $$( {}, '<div></div>' );
obj.model.set( { immutableValue: [ 'immutable' ] } );
var extendedObj = $$( obj, {
  controller: {
    'create': function() {
      var iV = this.model.get( 'immutableValue' );
      iV.push( 'mutability' );
    } // create
  } // controller
}); // extendedObj
deepEqual( obj.model.get( 'immutableValue' ), [ 'immutable' ], 'prototype model should be immutable' );

extendedObj.model.set( { immutableValue: [ 'not immutable' ] } );

$$.document.append( extendedObj );

deepEqual( obj.model.get( 'immutableValue' ), [ 'immutable' ], 'prototype model should be immutable' );
arturadib commented 13 years ago

@tristanls Thanks for the great catch here, love those tests! It just so happens that there was a bug with how I extended model._data in the line right under your patch. It's now fixed, so all your tests pass.

PS: Extending all of prototype.model would be cumbersome as it would create copies of the model methods too (.set(), .get()). All we need is to extend the model _data. I mistyped that line that extended the model data so that's why there was a bug. Thanks again!

tristanls commented 13 years ago

That makes sense, I knew my fix felt off, but the tests passed so I didn't dig further. Good to know the correct fix, yeah, that makes more sense, thanks!