Merge pull request #18741 from emberjs/bugfix/mandatory-setters-dont-…

…clobber [BUGFIX release] Don't setup mandatory setters on array indexes
emberjs · Feb 28, 2020 · 4cfba41 · 4cfba41
2 parents fbdcb9c + 5aa4c61
commit 4cfba41
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 1 deletion.
diff --git a/packages/@ember/-internals/runtime/tests/system/object/create_test.js b/packages/@ember/-internals/runtime/tests/system/object/create_test.js
@@ -1,5 +1,5 @@
 import { getOwner, setOwner } from '@ember/-internals/owner';
-import { computed, Mixin, observer } from '@ember/-internals/metal';
+import { computed, Mixin, observer, addObserver, destroy } from '@ember/-internals/metal';
 import { DEBUG } from '@glimmer/env';
 import EmberObject from '../../../lib/system/object';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
@@ -51,6 +51,43 @@ moduleFor(
       }
     }
 
+    ['@test does not sets up separate mandatory setters on getters'](assert) {
+      if (DEBUG) {
+        let MyClass = EmberObject.extend({
+          get foo() {
+            return 'bar';
+          },
+          fooDidChange: observer('foo', function() {}),
+        });
+
+        let o = MyClass.create({});
+        assert.equal(o.get('foo'), 'bar');
+
+        let descriptor = Object.getOwnPropertyDescriptor(o, 'foo');
+        assert.ok(!descriptor, 'Mandatory setter was not setup');
+
+        // cleanup
+        o.destroy();
+      } else {
+        assert.expect(0);
+      }
+    }
+
+    ['@test does not sets up separate mandatory setters on arrays'](assert) {
+      if (DEBUG) {
+        let arr = [123];
+
+        addObserver(arr, 0, function() {});
+
+        let descriptor = Object.getOwnPropertyDescriptor(arr, 0);
+        assert.ok(!descriptor.set, 'Mandatory setter was not setup');
+
+        destroy(arr);
+      } else {
+        assert.expect(0);
+      }
+    }
+
     ['@test calls setUnknownProperty if defined'](assert) {
       let setUnknownPropertyCalled = false;
 

diff --git a/packages/@ember/-internals/utils/lib/mandatory-setter.ts b/packages/@ember/-internals/utils/lib/mandatory-setter.ts
@@ -14,6 +14,19 @@ export let setWithMandatorySetter:
 
 type PropertyDescriptorWithMeta = PropertyDescriptor & { hadOwnProperty?: boolean };
 
+function isElementKey(key: string | number | symbol) {
+  return typeof key === 'number' ? isPositiveInt(key) : isStringInt(key as string);
+}
+
+function isStringInt(str: string) {
+  let num = parseInt(str, 10);
+  return isPositiveInt(num) && str === String(num);
+}
+
+function isPositiveInt(num: number) {
+  return num >= 0 && num % 1 === 0;
+}
+
 if (DEBUG) {
   let SEEN_TAGS = new WeakSet();
 
@@ -34,6 +47,10 @@ if (DEBUG) {
 
     SEEN_TAGS!.add(tag);
 
+    if (Array.isArray(obj) && isElementKey(keyName)) {
+      return;
+    }
+
     let desc = (lookupDescriptor(obj, keyName) as PropertyDescriptorWithMeta) || {};
 
     if (desc.get || desc.set) {