Skip to content

Commit

Permalink
[FEAT] Implements the Computed Property Modifier deprecation RFCs
Browse files Browse the repository at this point in the history 
- Deprecates `.property()`
- Deprecates `.volatile()`
- Deprecates `clobberSet` internally
- Keeps injected properties and the `store` property on routes
  overridable for testing purposes
- Rewrites lots of usages in tests. For some reason `volatile` was
  frequently used in tests when it was not needed.
- Adds `additionalDependentKeys` array to `map`, `filter` and `sort`
  array functions. `sort` was overlooked in RFC, but suffers from the
  same problem so it shouldn't need an additional RFC.
- Adds tests for `additionalDependentKeys`
  • Loading branch information
Chris Garrett committed Jan 11, 2019
1 parent a247b08 commit f3693d6
Show file tree
Hide file tree
Showing 15 changed files with 719 additions and 432 deletions.
39 changes: 37 additions & 2 deletions packages/@ember/-internals/metal/lib/computed.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { meta as metaFor, peekMeta } from '@ember/-internals/meta';
import { inspect } from '@ember/-internals/utils';
import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
import { assert, warn } from '@ember/debug';
import { assert, deprecate, warn } from '@ember/debug';
import EmberError from '@ember/error';
import {
getCachedValueFor,
Expand Down Expand Up @@ -224,6 +224,16 @@ class ComputedProperty extends Descriptor implements DescriptorWithDependentKeys
@public
*/
volatile(): ComputedProperty {
deprecate(
'Setting a computed property as volatile has been deprecated. Instead, consider using a native getter with native class syntax.',
false,
{
id: 'computed-property.volatile',
until: '4.0.0',
url: 'https://emberjs.com/deprecations/v3.x#toc_computed-property-volatile',
}
);

this._volatile = true;
return this;
}
Expand Down Expand Up @@ -291,6 +301,21 @@ class ComputedProperty extends Descriptor implements DescriptorWithDependentKeys
@public
*/
property(...passedArgs: string[]): ComputedProperty {
deprecate(
'Setting dependency keys using the `.property()` modifier has been deprecated. Pass the dependency keys directly to computed as arguments instead. If you are using `.property()` on a computed property macro, consider refactoring your macro to receive additional dependent keys in its initial declaration.',
false,
{
id: 'computed-property.property',
until: '4.0.0',
url: 'https://emberjs.com/deprecations/v3.x#toc_computed-property-property',
}
);

this._property(...passedArgs);
return this;
}

_property(...passedArgs: string[]): ComputedProperty {
let args: string[] = [];

function addArg(property: string): void {
Expand Down Expand Up @@ -450,6 +475,16 @@ 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.`,
false,
{
id: 'computed-property.override',
until: '4.0.0',
url: 'https://emberjs.com/deprecations/v3.x#toc_computed-property-override',
}
);

let cachedValue = getCachedValueFor(obj, keyName);
defineProperty(obj, keyName, null, cachedValue);
set(obj, keyName, value);
Expand Down Expand Up @@ -614,7 +649,7 @@ export default function computed(...args: (string | ComputedPropertyConfig)[]):
let cp = new ComputedProperty(func as ComputedPropertyConfig);

if (args.length > 0) {
cp.property(...(args as string[]));
cp._property(...(args as string[]));
}

return cp;
Expand Down
43 changes: 25 additions & 18 deletions packages/@ember/-internals/metal/lib/injected_property.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { getOwner } from '@ember/-internals/owner';
import { EMBER_MODULE_UNIFICATION } from '@ember/canary-features';
import { assert } from '@ember/debug';
import { ComputedProperty } from './computed';
import { defineProperty } from './properties';

export interface InjectedPropertyOptions {
source: string;
Expand Down Expand Up @@ -31,7 +32,7 @@ export default class InjectedProperty extends ComputedProperty {
readonly namespace: string | undefined;

constructor(type: string, name: string, options?: InjectedPropertyOptions) {
super(injectedPropertyGet);
super(injectedPropertyDesc);

this.type = type;
this.name = name;
Expand All @@ -54,22 +55,28 @@ export default class InjectedProperty extends ComputedProperty {
}
}

function injectedPropertyGet(this: any, keyName: string): any {
let desc = descriptorFor(this, keyName);
let owner = getOwner(this) || this.container; // fallback to `container` for backwards compat
const injectedPropertyDesc = {
get(this: any, keyName: string): any {
let desc = descriptorFor(this, keyName);
let owner = getOwner(this) || this.container; // fallback to `container` for backwards compat

assert(
`InjectedProperties should be defined with the inject computed property macros.`,
desc && desc.type
);
assert(
`Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.`,
Boolean(owner)
);
assert(
`InjectedProperties should be defined with the inject computed property macros.`,
desc && desc.type
);
assert(
`Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.`,
Boolean(owner)
);

let specifier = `${desc.type}:${desc.name || keyName}`;
return owner.lookup(specifier, {
source: desc.source,
namespace: desc.namespace,
});
}
let specifier = `${desc.type}:${desc.name || keyName}`;
return owner.lookup(specifier, {
source: desc.source,
namespace: desc.namespace,
});
},

set(this: any, keyName: string, value: any) {
defineProperty(this, keyName, null, value);
},
};
112 changes: 59 additions & 53 deletions packages/@ember/-internals/metal/tests/computed_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,15 @@ moduleFor(
}

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

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

set(obj, 'foo', 'boom');
set(obj, 'foo', 'boom');

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

['@test defining computed property should invoke property on set'](assert) {
Expand Down Expand Up @@ -399,13 +401,13 @@ moduleFor(
defineProperty(
obj,
'plusOne',
computed({
computed('foo', {
get() {},
set(key, value, oldValue) {
receivedOldValue = oldValue;
return value;
},
}).property('foo')
})
);

set(obj, 'plusOne', 1);
Expand Down Expand Up @@ -438,10 +440,10 @@ moduleFor(
defineProperty(
obj,
'foo',
computed({
computed('bar', {
get: getterAndSetter,
set: getterAndSetter,
}).property('bar')
})
);
}

Expand Down Expand Up @@ -478,11 +480,11 @@ moduleFor(
defineProperty(
obj,
'bar',
computed(function() {
computed('baz', function() {
count++;
get(this, 'baz');
return 'baz ' + count;
}).property('baz')
})
);

assert.equal(isWatching(obj, 'bar'), false, 'precond not watching dependent key');
Expand Down Expand Up @@ -515,15 +517,15 @@ moduleFor(
count++;
return 'bar ' + count;
};
defineProperty(obj, 'bar', computed({ get: func, set: func }).property('foo'));
defineProperty(obj, 'bar', computed('foo', { get: func, set: func }));

defineProperty(
obj,
'foo',
computed(function() {
computed('bar', function() {
count++;
return 'foo ' + count;
}).property('bar')
})
);

assert.equal(get(obj, 'foo'), 'foo 1', 'get once');
Expand All @@ -543,10 +545,10 @@ moduleFor(
defineProperty(
obj,
'foo',
computed(function() {
computed('baz', function() {
count++;
return 'baz ' + count;
}).property('baz')
})
);

assert.equal(
Expand All @@ -570,10 +572,10 @@ moduleFor(
defineProperty(
obj,
'foo',
computed(function() {
computed('qux.{bar,baz}', function() {
count++;
return 'foo ' + count;
}).property('qux.{bar,baz}')
})
);

assert.equal(get(obj, 'foo'), 'foo 1', 'get once');
Expand All @@ -598,10 +600,10 @@ moduleFor(
defineProperty(
obj,
'roo',
computed(function() {
computed('fee.{bar, baz,bop , }', function() {
count++;
return 'roo ' + count;
}).property('fee.{bar, baz,bop , }')
})
);
}, /cannot contain spaces/);
}
Expand Down Expand Up @@ -656,7 +658,7 @@ moduleFor(

['@test depending on simple chain'](assert) {
// assign computed property
defineProperty(obj, 'prop', computed(func).property('foo.bar.baz.biff'));
defineProperty(obj, 'prop', computed('foo.bar.baz.biff', func));

assert.equal(get(obj, 'prop'), 'BIFF 1');

Expand Down Expand Up @@ -699,7 +701,7 @@ moduleFor(

['@test chained dependent keys should evaluate computed properties lazily'](assert) {
defineProperty(obj.foo.bar, 'b', computed(func));
defineProperty(obj.foo, 'c', computed(function() {}).property('bar.b'));
defineProperty(obj.foo, 'c', computed('bar.b', function() {}));
assert.equal(count, 0, 'b should not run');
}
}
Expand Down Expand Up @@ -738,21 +740,23 @@ moduleFor(
}

['@test setter can be omited'](assert) {
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();
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();

assert.ok(testObj.get('aInt') === 1, 'getter works');
assert.ok(testObj.get('a') === '1');
testObj.set('aInt', '123');
assert.ok(testObj.get('aInt') === '123', 'cp has been updated too');
assert.ok(testObj.get('aInt') === 1, 'getter works');
assert.ok(testObj.get('a') === '1');
testObj.set('aInt', '123');
assert.ok(testObj.get('aInt') === '123', 'cp has been updated too');
}, /The aInt computed property was just overriden/);
}

['@test getter can be omited'](assert) {
Expand Down Expand Up @@ -851,7 +855,7 @@ moduleFor(
defineProperty(
obj,
'fullName',
computed({
computed('firstName', 'lastName', {
get() {
return get(this, 'firstName') + ' ' + get(this, 'lastName');
},
Expand All @@ -861,7 +865,7 @@ moduleFor(
set(this, 'lastName', values[1]);
return value;
},
}).property('firstName', 'lastName')
})
);

let fullNameDidChange = 0;
Expand Down Expand Up @@ -900,15 +904,15 @@ moduleFor(
defineProperty(
obj,
'plusOne',
computed({
computed('foo', {
get() {
return get(this, 'foo') + 1;
},
set(key, value) {
set(this, 'foo', value);
return value + 1;
},
}).property('foo')
})
);

let plusOneDidChange = 0;
Expand Down Expand Up @@ -936,24 +940,26 @@ moduleFor(
'computed - default setter',
class extends AbstractTestCase {
["@test when setting a value on a computed property that doesn't handle sets"](assert) {
let obj = {};
let observerFired = false;
expectDeprecation(() => {
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));

set(obj, 'foo', 'bar');
set(obj, 'foo', 'bar');

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');
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./);
}
}
);
Expand Down
Loading

0 comments on commit f3693d6

Please sign in to comment.