'this' now properly binds to the private member object of the instance for getters/setters
Getters/setters did not get much attention during the initial development of ease.js, simply because there was such a strong focus on pre-ES5 compatibility---ease.js was created for a project that strongly required it. Given that, getters/setters were not used, since those are ES5 features. As such, I find that two things have happened: 1. There was little incentive to provide a proper implementation; even though I noticed the issues during the initial development, they were left unresolved and were then forgotten about as the project lay dormant for a while. 2. The project was dormant because it was working as intended (sure, there are still things on the TODO-list feature-wise). Since getters/setters were unused in the project for which ease.js was created, the bug was never found and so never addressed. That said, I now am using getters/setters in a project with ease.js and noticed a very odd bug that could not be explained by that project's implementation. Sure enough, it was an ease.js issue and this commit resolves it. Now, there is more to be said about this commit. Mainly, it should be noted that MemberBuilder.buildGetterSetter, when compared with its method counterpart (buildMethod) is incomplete---it does not properly address overrides, the abstract keyword, proxies or the possibility of method hiding. This is certainly something that I will get to, but I want to get this fix out as soon as I can. Since overriding ES5 getters/setters (rather than explicit methods) is more likely to be a rarity, and since a partial fix is better than no fix, this will likely be tagged immediately and a further fix will follow in the (hopefully near) future. (This is an interesting example of how glaring bugs manage to slip through the cracks, even when the developer is initially aware of them.)perfodd
parent
8b74ed9f1b
commit
b4fe08292f
|
@ -492,10 +492,15 @@ exports.prototype.buildMembers = function buildMembers(
|
||||||
|
|
||||||
getset: function( name, get, set, keywords )
|
getset: function( name, get, set, keywords )
|
||||||
{
|
{
|
||||||
var dest = ( keywordStatic( keywords ) ) ? smethods : members;
|
var dest = ( keywordStatic( keywords ) ) ? smethods : members,
|
||||||
|
is_static = keywordStatic( keywords ),
|
||||||
|
instLookup = ( ( is_static )
|
||||||
|
? staticInstLookup
|
||||||
|
: exports.getMethodInstance
|
||||||
|
);
|
||||||
|
|
||||||
_self._memberBuilder.buildGetterSetter(
|
_self._memberBuilder.buildGetterSetter(
|
||||||
dest, null, name, get, set, keywords, base
|
dest, null, name, get, set, keywords, instLookup, class_id, base
|
||||||
);
|
);
|
||||||
},
|
},
|
||||||
|
|
||||||
|
|
|
@ -211,6 +211,9 @@ exports.buildProp = function( members, meta, name, value, keywords, base )
|
||||||
* Copies a getter/setter to the appropriate member prototype, depending on
|
* Copies a getter/setter to the appropriate member prototype, depending on
|
||||||
* visibility, and assigns necessary metadata from keywords
|
* visibility, and assigns necessary metadata from keywords
|
||||||
*
|
*
|
||||||
|
* TODO: This should essentially mirror buildMethod with regards to overrides,
|
||||||
|
* proxies, etc.
|
||||||
|
*
|
||||||
* @param {!__visobj} members
|
* @param {!__visobj} members
|
||||||
* @param {!Object} meta metadata container
|
* @param {!Object} meta metadata container
|
||||||
* @param {string} name getter name
|
* @param {string} name getter name
|
||||||
|
@ -219,6 +222,10 @@ exports.buildProp = function( members, meta, name, value, keywords, base )
|
||||||
*
|
*
|
||||||
* @param {!Object.<boolean>} keywords parsed keywords
|
* @param {!Object.<boolean>} keywords parsed keywords
|
||||||
*
|
*
|
||||||
|
* @param {Function} instCallback function to call in order to retrieve
|
||||||
|
* object to bind 'this' keyword to
|
||||||
|
*
|
||||||
|
* @param {number} cid class id
|
||||||
* @param {Object=} base optional base object to scan
|
* @param {Object=} base optional base object to scan
|
||||||
*
|
*
|
||||||
* @return {undefined}
|
* @return {undefined}
|
||||||
|
@ -227,7 +234,7 @@ exports.buildProp = function( members, meta, name, value, keywords, base )
|
||||||
* @suppress {checkTypes}
|
* @suppress {checkTypes}
|
||||||
*/
|
*/
|
||||||
exports.buildGetterSetter = function(
|
exports.buildGetterSetter = function(
|
||||||
members, meta, name, get, set, keywords, base
|
members, meta, name, get, set, keywords, instCallback, cid, base
|
||||||
)
|
)
|
||||||
{
|
{
|
||||||
var prev_data = scanMembers( members, name, base ),
|
var prev_data = scanMembers( members, name, base ),
|
||||||
|
@ -241,14 +248,24 @@ exports.buildGetterSetter = function(
|
||||||
name, {}, keywords, prev_data, prev_keywords
|
name, {}, keywords, prev_data, prev_keywords
|
||||||
);
|
);
|
||||||
|
|
||||||
|
if ( get )
|
||||||
|
{
|
||||||
|
get = this._overrideMethod( null, get, instCallback, cid );
|
||||||
|
|
||||||
|
// ensure we store the keywords *after* the override, otherwise they
|
||||||
|
// will be assigned to the wrapped function (the getter)
|
||||||
get.___$$keywords$$ = keywords;
|
get.___$$keywords$$ = keywords;
|
||||||
|
}
|
||||||
|
|
||||||
Object.defineProperty(
|
Object.defineProperty(
|
||||||
getMemberVisibility( members, keywords, name ),
|
getMemberVisibility( members, keywords, name ),
|
||||||
name,
|
name,
|
||||||
{
|
{
|
||||||
get: get,
|
get: get,
|
||||||
set: set,
|
set: ( set )
|
||||||
|
? this._overrideMethod( null, set, instCallback, cid )
|
||||||
|
: set,
|
||||||
|
|
||||||
enumerable: true,
|
enumerable: true,
|
||||||
configurable: false,
|
configurable: false,
|
||||||
}
|
}
|
||||||
|
|
|
@ -349,7 +349,7 @@ exports.prototype.validateGetterSetter = function(
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
if ( !( prev_keywords[ 'virtual' ] ) )
|
if ( !( prev_keywords && prev_keywords[ 'virtual' ] ) )
|
||||||
{
|
{
|
||||||
throw TypeError(
|
throw TypeError(
|
||||||
"Cannot override non-virtual getter/setter '" + name + "'"
|
"Cannot override non-virtual getter/setter '" + name + "'"
|
||||||
|
|
|
@ -77,6 +77,11 @@ Object.defineProperty( sub_foo_def, 'override bar', {
|
||||||
enumerable: true,
|
enumerable: true,
|
||||||
} );
|
} );
|
||||||
|
|
||||||
|
// this is important since the system may freeze the object, so we must have
|
||||||
|
// declared it in advance
|
||||||
|
foo_def.bar2 = '';
|
||||||
|
|
||||||
|
|
||||||
var Foo = Class.extend( foo_def ),
|
var Foo = Class.extend( foo_def ),
|
||||||
SubFoo = Foo.extend( sub_foo_def );
|
SubFoo = Foo.extend( sub_foo_def );
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue