Skip to content

Commit

Permalink
[BUGFIX] Don't setup mandatory setters on array indexes
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Chris Garrett committed Feb 27, 2020
1 parent 1f08044 commit 279217c
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getOwner, setOwner } from '@ember/-internals/owner';
import { computed, Mixin, observer } from '@ember/-internals/metal';
import { computed, Mixin, observer, addObserver } from '@ember/-internals/metal';
import { DEBUG } from '@glimmer/env';
import EmberObject from '../../../lib/system/object';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
Expand Down Expand Up @@ -49,6 +49,38 @@ 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');
} 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');
} else {
assert.expect(0);
}
}

['@test calls setUnknownProperty if defined'](assert) {
let setUnknownPropertyCalled = false;

Expand Down
17 changes: 17 additions & 0 deletions packages/@ember/-internals/utils/lib/mandatory-setter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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) {
Expand Down

0 comments on commit 279217c

Please sign in to comment.