From 078889fcc7149533a55ee4790db97c2143f57c98 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Mon, 15 Jun 2020 18:08:54 +0100 Subject: [PATCH 1/7] fix expando set --- src/can-observable-array.js | 11 +++++++++-- test/class-field-test.js | 7 +++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index f4d336b..0c4c418 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -59,8 +59,15 @@ class ObservableArray extends MixedInArray { const props = target.constructor.props; let value = descriptor.value; - if (!value && typeof descriptor.get === 'function') { - value = descriptor.get(); + if (!value) { + if (typeof descriptor.get === 'function') { + // Try to read the value with a getter + try { + value = descriptor.get(); + } catch (error) { + return mixins.expando(target, prop, value); + } + } } if (value) { diff --git a/test/class-field-test.js b/test/class-field-test.js index e33d739..113d546 100644 --- a/test/class-field-test.js +++ b/test/class-field-test.js @@ -74,4 +74,11 @@ QUnit.test('handle descriptor getter', function(assert) { }); foo.bar = 'Hola'; +}); + +QUnit.test('set should work', function(assert) { + class Foo extends ObservableArray{} + const foo = new Foo(); + foo.set("count", 3); + assert.equal(foo.count, 3); }); \ No newline at end of file From ebfadb13600d487038e2724d90b68d7b37db11b1 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Mon, 15 Jun 2020 18:45:24 +0100 Subject: [PATCH 2/7] Update the fix --- src/can-observable-array.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index 0c4c418..c232f6a 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -61,10 +61,9 @@ class ObservableArray extends MixedInArray { if (!value) { if (typeof descriptor.get === 'function') { - // Try to read the value with a getter - try { - value = descriptor.get(); - } catch (error) { + // Try to read the value with a getter + value = descriptor.get.call(target); + if (!value) { return mixins.expando(target, prop, value); } } From fff4cc11211599d38aebae52aa096476206e9d8e Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Mon, 15 Jun 2020 19:44:38 +0100 Subject: [PATCH 3/7] Handle class fields the right way --- src/can-observable-array.js | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index c232f6a..537c670 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -21,6 +21,7 @@ const localOnPatchesSymbol = "can.patches"; const onKeyValueSymbol = Symbol.for("can.onKeyValue"); const offKeyValueSymbol = Symbol.for("can.offKeyValue"); const metaSymbol = Symbol.for("can.meta"); +const inSetupSymbol = Symbol.for("can.initializing") function isListLike(items) { return canReflect.isListLike(items) && typeof items !== "string"; @@ -55,28 +56,25 @@ class ObservableArray extends MixedInArray { if ('items' === prop) { throw new Error('ObservableArray does not support a class field named items. Try using a different name or using static items'); } - - const props = target.constructor.props; - let value = descriptor.value; - if (!value) { - if (typeof descriptor.get === 'function') { - // Try to read the value with a getter + // Handle class fields + if (!target[inSetupSymbol]) { + let value = descriptor.value; + if (!value && typeof descriptor.get === 'function') { value = descriptor.get.call(target); - if (!value) { - return mixins.expando(target, prop, value); - } } - } - - if (value) { - if (props && props[prop]) { - obj[prop] = value; - return true; + + if (value) { + const props = target.constructor.props; + if (props && props[prop]) { + obj[prop] = value; + return true; + } + // Make the property observable + return mixins.expando(target, prop, value); } - // Make the property observable - return mixins.expando(target, prop, value); } + return Reflect.defineProperty(target, prop, descriptor); } }); } From 110f1d1cabdf277fa621b629e140eb4699108a69 Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Mon, 15 Jun 2020 20:44:04 +0100 Subject: [PATCH 4/7] more simple solution --- src/can-observable-array.js | 22 ++++++---------------- test/class-field-test.js | 26 -------------------------- 2 files changed, 6 insertions(+), 42 deletions(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index 537c670..e14c5da 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -21,7 +21,7 @@ const localOnPatchesSymbol = "can.patches"; const onKeyValueSymbol = Symbol.for("can.onKeyValue"); const offKeyValueSymbol = Symbol.for("can.offKeyValue"); const metaSymbol = Symbol.for("can.meta"); -const inSetupSymbol = Symbol.for("can.initializing") +const inSetupSymbol = Symbol.for("can.initializing"); function isListLike(items) { return canReflect.isListLike(items) && typeof items !== "string"; @@ -48,7 +48,7 @@ class ObservableArray extends MixedInArray { for(let i = 0, len = items && items.length; i < len; i++) { this[i] = convertItem(new.target, items[i]); } - const obj = this; + // Define class fields observables //and return the proxy return new Proxy(this, { @@ -58,22 +58,12 @@ class ObservableArray extends MixedInArray { } // Handle class fields - if (!target[inSetupSymbol]) { + if (target[inSetupSymbol] !== false) { let value = descriptor.value; - if (!value && typeof descriptor.get === 'function') { - value = descriptor.get.call(target); - } - - if (value) { - const props = target.constructor.props; - if (props && props[prop]) { - obj[prop] = value; - return true; - } - // Make the property observable - return mixins.expando(target, prop, value); - } + // Make the property observable + return mixins.expando(target, prop, value); } + // fall through as if there were not a `defineProperty` trap return Reflect.defineProperty(target, prop, descriptor); } }); diff --git a/test/class-field-test.js b/test/class-field-test.js index 113d546..992b0ef 100644 --- a/test/class-field-test.js +++ b/test/class-field-test.js @@ -50,32 +50,6 @@ QUnit.test('Throws on class field property named items', function (assert) { }); -QUnit.test('handle descriptor getter', function(assert) { - assert.expect(4); - - const foo = new ObservableArray(); - - let _bar = "Hello"; - Object.defineProperty(foo, "bar", { - get() { - return _bar; - }, - set(val) { - _bar = val; - } - }); - - assert.equal(foo.bar, 'Hello'); - - foo.on('bar', function (ev, newVal, oldVal) { - assert.equal(oldVal, 'Hello', 'Old value is correct'); - assert.equal(newVal, 'Hola', 'Value is updated'); - assert.ok(ev, 'The class field is observable'); - }); - - foo.bar = 'Hola'; -}); - QUnit.test('set should work', function(assert) { class Foo extends ObservableArray{} const foo = new Foo(); From 0b88a123fd589dcc232230bf3f45fd7c1dc80c1f Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Tue, 16 Jun 2020 17:40:48 +0100 Subject: [PATCH 5/7] Change how to check for initialization symbol --- src/can-observable-array.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index e14c5da..dd5d462 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -58,7 +58,7 @@ class ObservableArray extends MixedInArray { } // Handle class fields - if (target[inSetupSymbol] !== false) { + if (target[inSetupSymbol] === false) { let value = descriptor.value; // Make the property observable return mixins.expando(target, prop, value); From 6f1ff7286a1d93179662da67bfe1886c19fbd8cf Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Tue, 16 Jun 2020 20:31:10 +0100 Subject: [PATCH 6/7] Update the fix and handle props with same name --- src/can-observable-array.js | 19 ++++++++++--------- test/class-field-test.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index dd5d462..44ade26 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -21,7 +21,6 @@ const localOnPatchesSymbol = "can.patches"; const onKeyValueSymbol = Symbol.for("can.onKeyValue"); const offKeyValueSymbol = Symbol.for("can.offKeyValue"); const metaSymbol = Symbol.for("can.meta"); -const inSetupSymbol = Symbol.for("can.initializing"); function isListLike(items) { return canReflect.isListLike(items) && typeof items !== "string"; @@ -48,7 +47,7 @@ class ObservableArray extends MixedInArray { for(let i = 0, len = items && items.length; i < len; i++) { this[i] = convertItem(new.target, items[i]); } - + const instance = this; // Define class fields observables //and return the proxy return new Proxy(this, { @@ -57,14 +56,16 @@ class ObservableArray extends MixedInArray { throw new Error('ObservableArray does not support a class field named items. Try using a different name or using static items'); } - // Handle class fields - if (target[inSetupSymbol] === false) { - let value = descriptor.value; - // Make the property observable - return mixins.expando(target, prop, value); + // Don't overwrite static props + // that share the same name with a class field + const props = target.constructor.props; + if (props && props[prop]) { + instance[prop] = descriptor.value; + return true; } - // fall through as if there were not a `defineProperty` trap - return Reflect.defineProperty(target, prop, descriptor); + + let value = descriptor.value; + return mixins.expando(target, prop, value); } }); } diff --git a/test/class-field-test.js b/test/class-field-test.js index 992b0ef..2f7e324 100644 --- a/test/class-field-test.js +++ b/test/class-field-test.js @@ -55,4 +55,38 @@ QUnit.test('set should work', function(assert) { const foo = new Foo(); foo.set("count", 3); assert.equal(foo.count, 3); +}); + +QUnit.test('Class fields should not overwrite static props', function (assert) { + const done = assert.async(); + assert.expect(5); + + class MyList extends ObservableArray{ + /* jshint ignore:start */ + greetings = 'Hello'; + /* jshint ignore:end */ + static get props() { + return { + greetings: 'Bonjour' + }; + } + } + + const aList = new MyList(); + + assert.equal(aList.greetings, 'Hello', 'Default valus is correct'); + aList.on('greetings', function (ev, newVal, oldVal) { + assert.equal(oldVal, 'Hello', 'Old value is correct'); + assert.equal(newVal, 'Hola', 'Value is updated'); + assert.ok(ev, 'The class field is observable'); + done(); + }); + + aList.greetings = 'Hola'; + + try { + aList.greetings = {foo: 'bar'}; + } catch (error) { + assert.ok(error, 'Error thrown on the wrong type'); + } }); \ No newline at end of file From 65ed99ad38ead91601212f1bd6a840e58426872e Mon Sep 17 00:00:00 2001 From: Cherif BOUCHELAGHEM Date: Tue, 16 Jun 2020 20:33:23 +0100 Subject: [PATCH 7/7] Code readability update --- src/can-observable-array.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/can-observable-array.js b/src/can-observable-array.js index 44ade26..73fd466 100644 --- a/src/can-observable-array.js +++ b/src/can-observable-array.js @@ -55,16 +55,17 @@ class ObservableArray extends MixedInArray { if ('items' === prop) { throw new Error('ObservableArray does not support a class field named items. Try using a different name or using static items'); } + + let value = descriptor.value; // Don't overwrite static props // that share the same name with a class field const props = target.constructor.props; if (props && props[prop]) { - instance[prop] = descriptor.value; + instance[prop] = value; return true; } - let value = descriptor.value; return mixins.expando(target, prop, value); } });