From 984a14b087526b6f45e6dcb99bdf6b6808d284ef Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sun, 13 Mar 2011 14:51:40 -0400 Subject: [PATCH 1/5] Added more detailed documentation regarding the class_instance object --- lib/class.js | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/class.js b/lib/class.js index 3aae42e..a081056 100644 --- a/lib/class.js +++ b/lib/class.js @@ -39,10 +39,23 @@ var class_meta = {}; /** * Stores class instance visibility object * - * For each instance id, an object exists that contains the private and - * protected members. + * An entry in this table exists for each instance, with the instance id (iid) + * as the key. For each instance, there is always a base. The base will contain + * a proxy to the public members on the instance itself. The base will also + * contain all protected members. * - * @type {Object.} + * Atop the base object is a private member object, with the base as its + * prototype. There exists a private member object for the instance itself and + * one for each supertype. This is stored by the class id (cid) as the key. This + * permits the private member object associated with the class of the method + * call to be bound to that method. For example, if a parent method is called, + * that call must be invoked in the context of the parent, so the private + * members of the parent must be made available. + * + * The resulting structure looks something like this: + * class_instance = { iid: { cid: {} } } + * + * @type {Object.>} */ var class_instance = {}; From e1bb48a8d9742d00fb8d58d052e62d8eb96ea4f6 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sun, 13 Mar 2011 15:39:14 -0400 Subject: [PATCH 2/5] Added visibility test to ensure supertypes do not have access to private members of subtypes when invoked --- test/test-class-visibility.js | 54 +++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index 5c82bf2..2179874 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -54,6 +54,15 @@ var common = require( './common' ), }, + /** + * Does the same as the above, but we won't override this one + */ + 'public nonOverrideGetProp': function( name ) + { + return this[ name ]; + }, + + /** * Allows us to set a value from within the class */ @@ -116,6 +125,9 @@ var common = require( './common' ), // protected/private properties (for testing purposes) return this[ name ]; }, + + + 'private myOwnPrivateFoo': function() {}, }), sub_foo = SubFoo(), @@ -442,3 +454,45 @@ var common = require( './common' ), ); } )(); + +/** + * When a parent method is invoked, the parent should not be given access to the + * private members of the invoking subtype. Why? + * + * This is not a matter of whether or not this is possible to do. In fact it's + * relatively simple to implement. The issue is whether or not it makes sense. + * Consider a compiled language. Let's say Foo and SubFoo (as defined in this + * test case) were written in C++. Should Foo have access to a private property + * on SubFoo when it is overridden? + * + * No - that doesn't make sense. The private member is not a member of Foo and + * therefore Foo would fail to even compile. Alright, but we don't have such a + * restriction in our case. So why not implement it? + * + * Proponents of such an implementation are likely thinking of the act of + * inheriting methods as a copy/paste type of scenario. If we inherit public + * method baz(), and it were a copy/paste type of situation, then surely baz() + * would have access to all of SubFoo's private members. But that is not the + * case. Should baz() be defined as a member of Foo, then its scope is + * restricted to Foo and its supertypes. That is not how OO works. It is /not/ + * copy/paste. It is inheriting functionality. + */ +( function testParentsShouldNotHaveAccessToPrivateMembersOfSubtypes() +{ + // property + assert.equal( + sub_foo.nonOverrideGetProp( '_pfoo' ), + undefined, + "Parent should not have access to private properties of subtype when " + + "a parent method is invoked" + ); + + // member + assert.equal( + sub_foo.nonOverrideGetProp( '_myOwnPrivateFoo' ), + undefined, + "Parent should not have access to private methods of subtype when " + + "a parent method is invoked" + ); +} )(); + From 9acedf6e91885c8a98413f75c59a0dc78a7a9bee Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sun, 13 Mar 2011 15:47:31 -0400 Subject: [PATCH 3/5] Added test todo to TODO --- TODO | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TODO b/TODO index 0c9e606..db9997a 100644 --- a/TODO +++ b/TODO @@ -8,6 +8,9 @@ - Disallow member redeclaration in definition - Permit binding on class methods - Provide ability to free class from memory (class data stored in internal vars) + - Move tests to directly test propobj + - Was never done after refactoring. It's tested as a consequence of being + used in the class module. Member Keywords - Restrictions; throw exceptions when unknown keywords are used From d05652f8802239a763ebd391bf5ec78b0abcd811 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sun, 13 Mar 2011 21:47:40 -0400 Subject: [PATCH 4/5] Fixed visibility issues in IE6 - Wasn't properly falling back --- lib/propobj.js | 11 ++++++++--- test/test-class-visibility.js | 7 +++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/propobj.js b/lib/propobj.js index 7ea372d..38d59e6 100644 --- a/lib/propobj.js +++ b/lib/propobj.js @@ -46,12 +46,17 @@ var util = require( './util' ), */ exports.setup = function( dest, properties, methods ) { + var obj = dest; + // this constructor is an extra layer atop of the destination object, which // will contain the private methods - var obj_ctor = function() {}; - obj_ctor.prototype = dest; + if ( defprop ) + { + var obj_ctor = function() {}; + obj_ctor.prototype = dest; - var obj = new obj_ctor(); + obj = new obj_ctor(); + } // initialize each of the properties for this instance to // ensure we're not sharing references to prototype values diff --git a/test/test-class-visibility.js b/test/test-class-visibility.js index 2179874..72c72a3 100644 --- a/test/test-class-visibility.js +++ b/test/test-class-visibility.js @@ -479,6 +479,13 @@ var common = require( './common' ), */ ( function testParentsShouldNotHaveAccessToPrivateMembersOfSubtypes() { + // browsers that do not support the property proxy will not support + // encapsulating properties + if ( !( propobj.supportsPropProxy() ) ) + { + return; + } + // property assert.equal( sub_foo.nonOverrideGetProp( '_pfoo' ), From 9a135a064c1477dd29a769522345ce6bddeb5ae7 Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Sun, 13 Mar 2011 22:08:08 -0400 Subject: [PATCH 5/5] Added pre-ES5 test to ensure we can catch fallback bugs quickly between browser tests (preferably, before even committing) --- test/test-combine-pre-es5.js | 77 ++++++++++++++++++++++++++++++++++++ tools/combine | 2 +- 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 test/test-combine-pre-es5.js diff --git a/test/test-combine-pre-es5.js b/test/test-combine-pre-es5.js new file mode 100644 index 0000000..cbca10e --- /dev/null +++ b/test/test-combine-pre-es5.js @@ -0,0 +1,77 @@ +/** + * Tests combined file, attempting to emulate a pre-ECMAScript5 environment. + * This will ensure fallbacks will work properly on older browsers, such as IE6. + * + * This is /not/ an alternative to running the test suite in the browser of your + * choice. It is intended to catch errors early, to ensure bugs are not + * committed between browser tests. + * + * Copyright (C) 2010 Mike Gerwitz + * + * This file is part of ease.js. + * + * ease.js is free software: you can redistribute it and/or modify it under the + * terms of the GNU Lesser General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License + * for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program. If not, see . + * + * @author Mike Gerwitz + * @package test + */ + +var common = require( './common' ), + assert = require( 'assert' ), + Class = common.require( 'class' ), + Script = process.binding( 'evals' ).Script, + + // sandbox in which combined script will be run + sandbox = { + // stub document.write() so we don't blow up + document: { write: function() {} }, + }; + + +var file = 'ease-full.js'; + +// attempt to read the combined file +try +{ + var data = require( 'fs' ) + .readFileSync( ( __dirname + '/../build/' + file ), 'ascii' ); +} +catch ( e ) +{ + // if the file doesn't exit, just skip the test + console.log( + "Combined file not found. Test skipped. Please run `make combined`." + ); + process.exit( 0 ); +} + +// Let's take this bitch back in time (this is not a complete list, but +// satisfies what we need). +// +// It is important to note that we prepend this to the script that we'll be +// executing, because the script will be executed within a new scope. Any +// clobbering we do in our scope will not affect it, nor will any clobbering we +// do to it affect us. +data = "delete Object.defineProperty;" + + "delete Array.prototype.forEach;" + + data +; + +// run the script (if this fails to compile, the generated code is invalid) +var cmb_script = new Script( data ); +cmb_script.runInNewContext( sandbox ); + +// cross your fingers +sandbox.easejs.runTests(); + diff --git a/tools/combine b/tools/combine index db82305..79b49b8 100755 --- a/tools/combine +++ b/tools/combine @@ -103,7 +103,7 @@ if [ "$INC_TEST" ]; then \) \ -exec basename {} \; \ | sort \ - | grep -v 'test-\(combine\|index\).js' \ + | grep -v 'test-\(combine\(-pre-es5\)\?\|index\).js' \ ) # include test combine template