Skip to content

Commit

Permalink
review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
Chris Garrett committed Jan 11, 2019
1 parent f3693d6 commit 1e7d959
Show file tree
Hide file tree
Showing 8 changed files with 366 additions and 249 deletions.
4 changes: 2 additions & 2 deletions packages/@ember/-internals/metal/lib/computed.ts
@@ -1,5 +1,5 @@
import { meta as metaFor, peekMeta } from '@ember/-internals/meta';
import { inspect } from '@ember/-internals/utils';
import { inspect, toString } from '@ember/-internals/utils';
import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
import { assert, deprecate, warn } from '@ember/debug';
import EmberError from '@ember/error';
Expand Down Expand Up @@ -476,7 +476,7 @@ class ComputedProperty extends Descriptor implements DescriptorWithDependentKeys

clobberSet(obj: object, keyName: string, value: any): any {
deprecate(
`The ${keyName} computed property was just overriden. This removes the computed property and replaces it with a plain value, and has been deprecated. If you want this behavior, consider defining a setter which does it manually.`,
`The ${toString(obj)}#${keyName} computed property was just overriden. This removes the computed property and replaces it with a plain value, and has been deprecated. If you want this behavior, consider defining a setter which does it manually.`,
false,
{
id: 'computed-property.override',
Expand Down
72 changes: 38 additions & 34 deletions packages/@ember/-internals/metal/tests/computed_test.js
Expand Up @@ -77,15 +77,17 @@ moduleFor(
}

['@test can override volatile computed property'](assert) {
expectDeprecation(() => {
let obj = {};
let obj = {};

expectDeprecation(() => {
defineProperty(obj, 'foo', computed(function() {}).volatile());
}, 'Setting a computed property as volatile has been deprecated. Instead, consider using a native getter with native class syntax.');

expectDeprecation(() => {
set(obj, 'foo', 'boom');
}, /The \[object Object\]#foo computed property was just overriden./);

assert.equal(obj.foo, 'boom');
}, 'Setting a computed property as volatile has been deprecated. Instead, consider using a native getter with native class syntax.');
assert.equal(obj.foo, 'boom');
}

['@test defining computed property should invoke property on set'](assert) {
Expand Down Expand Up @@ -740,23 +742,25 @@ moduleFor(
}

['@test setter can be omited'](assert) {
expectDeprecation(() => {
let testObj = EmberObject.extend({
a: '1',
b: '2',
aInt: computed('a', {
get(keyName) {
assert.equal(keyName, 'aInt', 'getter receives the keyName');
return parseInt(this.get('a'));
},
}),
}).create();
let testObj = EmberObject.extend({
a: '1',
b: '2',
aInt: computed('a', {
get(keyName) {
assert.equal(keyName, 'aInt', 'getter receives the keyName');
return parseInt(this.get('a'));
},
}),
}).create();

assert.ok(testObj.get('aInt') === 1, 'getter works');
assert.ok(testObj.get('a') === '1');
assert.ok(testObj.get('aInt') === 1, 'getter works');
assert.ok(testObj.get('a') === '1');

expectDeprecation(() => {
testObj.set('aInt', '123');
assert.ok(testObj.get('aInt') === '123', 'cp has been updated too');
}, /The aInt computed property was just overriden/);
}, /The <\(unknown\):ember\d*>#aInt computed property was just overriden/);

assert.ok(testObj.get('aInt') === '123', 'cp has been updated too');
}

['@test getter can be omited'](assert) {
Expand Down Expand Up @@ -940,26 +944,26 @@ moduleFor(
'computed - default setter',
class extends AbstractTestCase {
["@test when setting a value on a computed property that doesn't handle sets"](assert) {
expectDeprecation(() => {
let obj = {};
let observerFired = false;
let obj = {};
let observerFired = false;

defineProperty(
obj,
'foo',
computed(function() {
return 'foo';
})
);
defineProperty(
obj,
'foo',
computed(function() {
return 'foo';
})
);

addObserver(obj, 'foo', null, () => (observerFired = true));
addObserver(obj, 'foo', null, () => (observerFired = true));

expectDeprecation(() => {
set(obj, 'foo', 'bar');
}, /The \[object Object\]#foo computed property was just overriden./);

assert.equal(get(obj, 'foo'), 'bar', 'The set value is properly returned');
assert.ok(typeof obj.foo === 'string', 'The computed property was removed');
assert.ok(observerFired, 'The observer was still notified');
}, /The foo computed property was just overriden./);
assert.equal(get(obj, 'foo'), 'bar', 'The set value is properly returned');
assert.ok(typeof obj.foo === 'string', 'The computed property was removed');
assert.ok(observerFired, 'The observer was still notified');
}
}
);
Expand Down
104 changes: 51 additions & 53 deletions packages/@ember/-internals/metal/tests/mixin/computed_test.js
Expand Up @@ -110,62 +110,60 @@ moduleFor(
}

['@test setter behavior works properly when overriding computed properties'](assert) {
expectDeprecation(() => {
let obj = {};

let MixinA = Mixin.create({
cpWithSetter2: computed(K),
cpWithSetter3: computed(K),
cpWithoutSetter: computed(K),
});

let cpWasCalled = false;

let MixinB = Mixin.create({
cpWithSetter2: computed({
get: K,
set() {
cpWasCalled = true;
},
}),

cpWithSetter3: computed({
get: K,
set() {
cpWasCalled = true;
},
}),

cpWithoutSetter: computed(function() {
let obj = {};

let MixinA = Mixin.create({
cpWithSetter2: computed(K),
cpWithSetter3: computed(K),
cpWithoutSetter: computed(K),
});

let cpWasCalled = false;

let MixinB = Mixin.create({
cpWithSetter2: computed({
get: K,
set() {
cpWasCalled = true;
}),
});

MixinA.apply(obj);
MixinB.apply(obj);

set(obj, 'cpWithSetter2', 'test');
assert.ok(
cpWasCalled,
'The computed property setter was called when defined with two args'
);
cpWasCalled = false;

set(obj, 'cpWithSetter3', 'test');
assert.ok(
cpWasCalled,
'The computed property setter was called when defined with three args'
);
cpWasCalled = false;
},
}),

cpWithSetter3: computed({
get: K,
set() {
cpWasCalled = true;
},
}),

cpWithoutSetter: computed(function() {
cpWasCalled = true;
}),
});

MixinA.apply(obj);
MixinB.apply(obj);

set(obj, 'cpWithSetter2', 'test');
assert.ok(cpWasCalled, 'The computed property setter was called when defined with two args');
cpWasCalled = false;

set(obj, 'cpWithSetter3', 'test');
assert.ok(
cpWasCalled,
'The computed property setter was called when defined with three args'
);
cpWasCalled = false;

expectDeprecation(() => {
set(obj, 'cpWithoutSetter', 'test');
assert.equal(
get(obj, 'cpWithoutSetter'),
'test',
'The default setter was called, the value is correct'
);
assert.ok(!cpWasCalled, 'The default setter was called, not the CP itself');
}, /The cpWithoutSetter computed property was just overriden./);
}, /The \[object Object\]#cpWithoutSetter computed property was just overriden./);

assert.equal(
get(obj, 'cpWithoutSetter'),
'test',
'The default setter was called, the value is correct'
);
assert.ok(!cpWasCalled, 'The default setter was called, not the CP itself');
}
}
);

0 comments on commit 1e7d959

Please sign in to comment.