Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] Implements the Computed Property Modifier deprecation RFCs #17470

Merged
merged 2 commits into from Jan 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 40 additions & 3 deletions packages/@ember/-internals/metal/lib/computed.ts
@@ -1,7 +1,7 @@
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, 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,18 @@ class ComputedProperty extends Descriptor implements DescriptorWithDependentKeys
}

clobberSet(obj: object, keyName: string, value: any): any {
deprecate(
`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',
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 +651,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
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);
},
};
60 changes: 35 additions & 25 deletions packages/@ember/-internals/metal/tests/computed_test.js
Expand Up @@ -79,9 +79,13 @@ moduleFor(
['@test can override volatile computed property'](assert) {
let obj = {};

defineProperty(obj, 'foo', computed(function() {}).volatile());
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.');

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

assert.equal(obj.foo, 'boom');
}
Expand Down Expand Up @@ -399,13 +403,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 +442,10 @@ moduleFor(
defineProperty(
obj,
'foo',
computed({
computed('bar', {
get: getterAndSetter,
set: getterAndSetter,
}).property('bar')
})
);
}

Expand Down Expand Up @@ -478,11 +482,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 +519,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 +547,10 @@ moduleFor(
defineProperty(
obj,
'foo',
computed(function() {
computed('baz', function() {
count++;
return 'baz ' + count;
}).property('baz')
})
);

assert.equal(
Expand All @@ -570,10 +574,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 +602,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 +660,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 +703,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 @@ -751,7 +755,11 @@ moduleFor(

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

expectDeprecation(() => {
testObj.set('aInt', '123');
}, /The <\(unknown\):ember\d*>#aInt computed property was just overriden/);

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

Expand Down Expand Up @@ -851,7 +859,7 @@ moduleFor(
defineProperty(
obj,
'fullName',
computed({
computed('firstName', 'lastName', {
get() {
return get(this, 'firstName') + ' ' + get(this, 'lastName');
},
Expand All @@ -861,7 +869,7 @@ moduleFor(
set(this, 'lastName', values[1]);
return value;
},
}).property('firstName', 'lastName')
})
);

let fullNameDidChange = 0;
Expand Down Expand Up @@ -900,15 +908,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 @@ -949,7 +957,9 @@ moduleFor(

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

set(obj, 'foo', 'bar');
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');
Expand Down
Expand Up @@ -154,7 +154,10 @@ moduleFor(
);
cpWasCalled = false;

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

assert.equal(
get(obj, 'cpWithoutSetter'),
'test',
Expand Down