Skip to content

Commit 937fb74

Browse files
Chris Garrettkategengler
Chris Garrett
authored andcommitted
[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. (cherry picked from commit 5aa4c61)
1 parent 072a146 commit 937fb74

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)