Skip to content

Commit 5aa4c61

Browse files
author
Chris Garrett
committed
[BUGFIX] Don't setup mandatory setters on array indexes
It turns out that removing items from an array (e.g. via `splice`) will also remove their descriptors, if any. Could be a browser bug, but I'm not sure we intended to add setters directly to arrays in the first place, and if we can't definitely manage them we probably shouldn't.
1 parent fbdcb9c commit 5aa4c61

File tree

2 files changed

+55
-1
lines changed

2 files changed

+55
-1
lines changed

packages/@ember/-internals/runtime/tests/system/object/create_test.js

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { getOwner, setOwner } from '@ember/-internals/owner';
2-
import { computed, Mixin, observer } from '@ember/-internals/metal';
2+
import { computed, Mixin, observer, addObserver, destroy } from '@ember/-internals/metal';
33
import { DEBUG } from '@glimmer/env';
44
import EmberObject from '../../../lib/system/object';
55
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
@@ -51,6 +51,43 @@ moduleFor(
5151
}
5252
}
5353

54+
['@test does not sets up separate mandatory setters on getters'](assert) {
55+
if (DEBUG) {
56+
let MyClass = EmberObject.extend({
57+
get foo() {
58+
return 'bar';
59+
},
60+
fooDidChange: observer('foo', function() {}),
61+
});
62+
63+
let o = MyClass.create({});
64+
assert.equal(o.get('foo'), 'bar');
65+
66+
let descriptor = Object.getOwnPropertyDescriptor(o, 'foo');
67+
assert.ok(!descriptor, 'Mandatory setter was not setup');
68+
69+
// cleanup
70+
o.destroy();
71+
} else {
72+
assert.expect(0);
73+
}
74+
}
75+
76+
['@test does not sets up separate mandatory setters on arrays'](assert) {
77+
if (DEBUG) {
78+
let arr = [123];
79+
80+
addObserver(arr, 0, function() {});
81+
82+
let descriptor = Object.getOwnPropertyDescriptor(arr, 0);
83+
assert.ok(!descriptor.set, 'Mandatory setter was not setup');
84+
85+
destroy(arr);
86+
} else {
87+
assert.expect(0);
88+
}
89+
}
90+
5491
['@test calls setUnknownProperty if defined'](assert) {
5592
let setUnknownPropertyCalled = false;
5693

packages/@ember/-internals/utils/lib/mandatory-setter.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,19 @@ export let setWithMandatorySetter:
1414

1515
type PropertyDescriptorWithMeta = PropertyDescriptor & { hadOwnProperty?: boolean };
1616

17+
function isElementKey(key: string | number | symbol) {
18+
return typeof key === 'number' ? isPositiveInt(key) : isStringInt(key as string);
19+
}
20+
21+
function isStringInt(str: string) {
22+
let num = parseInt(str, 10);
23+
return isPositiveInt(num) && str === String(num);
24+
}
25+
26+
function isPositiveInt(num: number) {
27+
return num >= 0 && num % 1 === 0;
28+
}
29+
1730
if (DEBUG) {
1831
let SEEN_TAGS = new WeakSet();
1932

@@ -34,6 +47,10 @@ if (DEBUG) {
3447

3548
SEEN_TAGS!.add(tag);
3649

50+
if (Array.isArray(obj) && isElementKey(keyName)) {
51+
return;
52+
}
53+
3754
let desc = (lookupDescriptor(obj, keyName) as PropertyDescriptorWithMeta) || {};
3855

3956
if (desc.get || desc.set) {

0 commit comments

Comments
 (0)