From f3693d66bb908fdeeed9108532751b5bb121dc69 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Thu, 10 Jan 2019 22:19:59 -0800 Subject: [PATCH 1/2] [FEAT] Implements the Computed Property Modifier deprecation RFCs - 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` --- .../@ember/-internals/metal/lib/computed.ts | 39 ++- .../-internals/metal/lib/injected_property.ts | 43 +-- .../-internals/metal/tests/computed_test.js | 112 +++---- .../metal/tests/mixin/computed_test.js | 105 ++++--- .../-internals/metal/tests/observer_test.js | 24 +- .../metal/tests/performance_test.js | 4 +- .../metal/tests/watching/is_watching_test.js | 4 +- .../-internals/routing/lib/system/route.ts | 291 ++++++++++-------- .../mixins/observable/observable_test.js | 182 ++++++----- .../mixins/observable/propertyChanges_test.js | 52 ++-- .../array_proxy/arranged_content_test.js | 4 +- .../tests/system/object/computed_test.js | 32 +- .../runtime/tests/system/object_proxy_test.js | 4 +- .../lib/computed/reduce_computed_macros.js | 115 +++++-- .../computed/reduce_computed_macros_test.js | 140 +++++++++ 15 files changed, 719 insertions(+), 432 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts index cf522637c3c..6142029941a 100644 --- a/packages/@ember/-internals/metal/lib/computed.ts +++ b/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 { 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, @@ -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; } @@ -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 { @@ -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); @@ -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; diff --git a/packages/@ember/-internals/metal/lib/injected_property.ts b/packages/@ember/-internals/metal/lib/injected_property.ts index cbd720b0ded..ee8cc64a90b 100644 --- a/packages/@ember/-internals/metal/lib/injected_property.ts +++ b/packages/@ember/-internals/metal/lib/injected_property.ts @@ -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; @@ -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; @@ -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); + }, +}; diff --git a/packages/@ember/-internals/metal/tests/computed_test.js b/packages/@ember/-internals/metal/tests/computed_test.js index 44034631676..f37e707710f 100644 --- a/packages/@ember/-internals/metal/tests/computed_test.js +++ b/packages/@ember/-internals/metal/tests/computed_test.js @@ -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) { @@ -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); @@ -438,10 +440,10 @@ moduleFor( defineProperty( obj, 'foo', - computed({ + computed('bar', { get: getterAndSetter, set: getterAndSetter, - }).property('bar') + }) ); } @@ -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'); @@ -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'); @@ -543,10 +545,10 @@ moduleFor( defineProperty( obj, 'foo', - computed(function() { + computed('baz', function() { count++; return 'baz ' + count; - }).property('baz') + }) ); assert.equal( @@ -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'); @@ -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/); } @@ -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'); @@ -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'); } } @@ -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) { @@ -851,7 +855,7 @@ moduleFor( defineProperty( obj, 'fullName', - computed({ + computed('firstName', 'lastName', { get() { return get(this, 'firstName') + ' ' + get(this, 'lastName'); }, @@ -861,7 +865,7 @@ moduleFor( set(this, 'lastName', values[1]); return value; }, - }).property('firstName', 'lastName') + }) ); let fullNameDidChange = 0; @@ -900,7 +904,7 @@ moduleFor( defineProperty( obj, 'plusOne', - computed({ + computed('foo', { get() { return get(this, 'foo') + 1; }, @@ -908,7 +912,7 @@ moduleFor( set(this, 'foo', value); return value + 1; }, - }).property('foo') + }) ); let plusOneDidChange = 0; @@ -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./); } } ); diff --git a/packages/@ember/-internals/metal/tests/mixin/computed_test.js b/packages/@ember/-internals/metal/tests/mixin/computed_test.js index b094bcd0aa5..1a9a07afd6c 100644 --- a/packages/@ember/-internals/metal/tests/mixin/computed_test.js +++ b/packages/@ember/-internals/metal/tests/mixin/computed_test.js @@ -110,57 +110,62 @@ moduleFor( } ['@test setter behavior works properly when overriding computed properties'](assert) { - 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() { + 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() { 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; - - 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'); + }), + }); + + 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; + + 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./); } } ); diff --git a/packages/@ember/-internals/metal/tests/observer_test.js b/packages/@ember/-internals/metal/tests/observer_test.js index 17e83ee48b2..ffa4ed59cf3 100644 --- a/packages/@ember/-internals/metal/tests/observer_test.js +++ b/packages/@ember/-internals/metal/tests/observer_test.js @@ -54,9 +54,9 @@ moduleFor( defineProperty( obj, 'foo', - computed(function() { + computed('bar', function() { return get(this, 'bar').toUpperCase(); - }).property('bar') + }) ); get(obj, 'foo'); @@ -139,17 +139,17 @@ moduleFor( defineProperty( obj, 'foo', - computed(function() { + computed('bar', function() { return get(this, 'bar').toLowerCase(); - }).property('bar') + }) ); defineProperty( obj, 'bar', - computed(function() { + computed('baz', function() { return get(this, 'baz').toUpperCase(); - }).property('baz') + }) ); mixin(obj, { @@ -205,17 +205,17 @@ moduleFor( defineProperty( obj, 'foo', - computed(function() { + computed('bar', function() { return get(this, 'bar').toLowerCase(); - }).property('bar') + }) ); defineProperty( obj, 'bar', - computed(function() { + computed('baz', function() { return get(this, 'baz').toUpperCase(); - }).property('baz') + }) ); mixin(obj, { @@ -690,14 +690,14 @@ moduleFor( defineProperty( obj, 'foo', - computed({ + computed('baz', { get: function() { return get(this, 'baz'); }, set: function(key, value) { return value; }, - }).property('baz') + }) ); let count = 0; diff --git a/packages/@ember/-internals/metal/tests/performance_test.js b/packages/@ember/-internals/metal/tests/performance_test.js index f88f074495a..a820ffeb8a0 100644 --- a/packages/@ember/-internals/metal/tests/performance_test.js +++ b/packages/@ember/-internals/metal/tests/performance_test.js @@ -30,10 +30,10 @@ moduleFor( defineProperty( obj, 'abc', - computed(function(key) { + computed('a', 'b', 'c', function(key) { cpCount++; return 'computed ' + key; - }).property('a', 'b', 'c') + }) ); get(obj, 'abc'); diff --git a/packages/@ember/-internals/metal/tests/watching/is_watching_test.js b/packages/@ember/-internals/metal/tests/watching/is_watching_test.js index 77321839554..19cbbed261b 100644 --- a/packages/@ember/-internals/metal/tests/watching/is_watching_test.js +++ b/packages/@ember/-internals/metal/tests/watching/is_watching_test.js @@ -61,7 +61,7 @@ moduleFor( testObserver( assert, (obj, key, fn) => { - defineProperty(obj, fn, computed(function() {}).property(key)); + defineProperty(obj, fn, computed(key, function() {})); get(obj, fn); }, (obj, key, fn) => defineProperty(obj, fn, null) @@ -72,7 +72,7 @@ moduleFor( testObserver( assert, (obj, key, fn) => { - defineProperty(obj, fn, computed(function() {}).property(key + '.bar')); + defineProperty(obj, fn, computed(key + '.bar', function() {})); get(obj, fn); }, (obj, key, fn) => defineProperty(obj, fn, null) diff --git a/packages/@ember/-internals/routing/lib/system/route.ts b/packages/@ember/-internals/routing/lib/system/route.ts index 2234211fb6a..babf671acce 100644 --- a/packages/@ember/-internals/routing/lib/system/route.ts +++ b/packages/@ember/-internals/routing/lib/system/route.ts @@ -1,4 +1,12 @@ -import { computed, get, getProperties, isEmpty, set, setProperties } from '@ember/-internals/metal'; +import { + computed, + defineProperty, + get, + getProperties, + isEmpty, + set, + setProperties, +} from '@ember/-internals/metal'; import { getOwner, Owner } from '@ember/-internals/owner'; import { A as emberA, @@ -2134,33 +2142,42 @@ Route.reopen(ActionHandler, Evented, { @type {Object} @private */ - store: computed(function(this: Route) { - let owner = getOwner(this); - let routeName = this.routeName; - let namespace = get(this, '_router.namespace'); - - return { - find(name: string, value: unknown) { - let modelClass: any = owner.factoryFor(`model:${name}`); - - assert( - `You used the dynamic segment ${name}_id in your route ${routeName}, but ${namespace}.${classify( - name - )} did not exist and you did not override your route's \`model\` hook.`, - Boolean(modelClass) - ); + store: computed({ + get(this: Route) { + let owner = getOwner(this); + let routeName = this.routeName; + let namespace = get(this, '_router.namespace'); + + return { + find(name: string, value: unknown) { + let modelClass: any = owner.factoryFor(`model:${name}`); + + assert( + `You used the dynamic segment ${name}_id in your route ${routeName}, but ${namespace}.${classify( + name + )} did not exist and you did not override your route's \`model\` hook.`, + Boolean(modelClass) + ); + + if (!modelClass) { + return; + } - if (!modelClass) { - return; - } + modelClass = modelClass.class; - modelClass = modelClass.class; + assert( + `${classify(name)} has no method \`find\`.`, + typeof modelClass.find === 'function' + ); - assert(`${classify(name)} has no method \`find\`.`, typeof modelClass.find === 'function'); + return modelClass.find(value); + }, + }; + }, - return modelClass.find(value); - }, - }; + set(key, value) { + defineProperty(this, key, null, value); + }, }), /** @@ -2168,128 +2185,134 @@ Route.reopen(ActionHandler, Evented, { @property _qp */ - _qp: computed(function(this: Route) { - let combinedQueryParameterConfiguration; + _qp: computed({ + get(this: Route) { + let combinedQueryParameterConfiguration; + + let controllerName = this.controllerName || this.routeName; + let owner = getOwner(this); + let controller = owner.lookup(`controller:${controllerName}`); + let queryParameterConfiguraton = get(this, 'queryParams'); + let hasRouterDefinedQueryParams = Object.keys(queryParameterConfiguraton).length > 0; + + if (controller) { + // the developer has authored a controller class in their application for + // this route find its query params and normalize their object shape them + // merge in the query params for the route. As a mergedProperty, + // Route#queryParams is always at least `{}` + + let controllerDefinedQueryParameterConfiguration = get(controller, 'queryParams') || {}; + let normalizedControllerQueryParameterConfiguration = normalizeControllerQueryParams( + controllerDefinedQueryParameterConfiguration + ); + combinedQueryParameterConfiguration = mergeEachQueryParams( + normalizedControllerQueryParameterConfiguration, + queryParameterConfiguraton + ); + } else if (hasRouterDefinedQueryParams) { + // the developer has not defined a controller but *has* supplied route query params. + // Generate a class for them so we can later insert default values + controller = generateController(owner, controllerName); + combinedQueryParameterConfiguration = queryParameterConfiguraton; + } - let controllerName = this.controllerName || this.routeName; - let owner = getOwner(this); - let controller = owner.lookup(`controller:${controllerName}`); - let queryParameterConfiguraton = get(this, 'queryParams'); - let hasRouterDefinedQueryParams = Object.keys(queryParameterConfiguraton).length > 0; - - if (controller) { - // the developer has authored a controller class in their application for - // this route find its query params and normalize their object shape them - // merge in the query params for the route. As a mergedProperty, - // Route#queryParams is always at least `{}` - - let controllerDefinedQueryParameterConfiguration = get(controller, 'queryParams') || {}; - let normalizedControllerQueryParameterConfiguration = normalizeControllerQueryParams( - controllerDefinedQueryParameterConfiguration - ); - combinedQueryParameterConfiguration = mergeEachQueryParams( - normalizedControllerQueryParameterConfiguration, - queryParameterConfiguraton - ); - } else if (hasRouterDefinedQueryParams) { - // the developer has not defined a controller but *has* supplied route query params. - // Generate a class for them so we can later insert default values - controller = generateController(owner, controllerName); - combinedQueryParameterConfiguration = queryParameterConfiguraton; - } + let qps = []; + let map = {}; + let propertyNames = []; - let qps = []; - let map = {}; - let propertyNames = []; + for (let propName in combinedQueryParameterConfiguration) { + if (!combinedQueryParameterConfiguration.hasOwnProperty(propName)) { + continue; + } - for (let propName in combinedQueryParameterConfiguration) { - if (!combinedQueryParameterConfiguration.hasOwnProperty(propName)) { - continue; - } + // to support the dubious feature of using unknownProperty + // on queryParams configuration + if (propName === 'unknownProperty' || propName === '_super') { + // possible todo: issue deprecation warning? + continue; + } - // to support the dubious feature of using unknownProperty - // on queryParams configuration - if (propName === 'unknownProperty' || propName === '_super') { - // possible todo: issue deprecation warning? - continue; - } + let desc = combinedQueryParameterConfiguration[propName]; + let scope = desc.scope || 'model'; + let parts; - let desc = combinedQueryParameterConfiguration[propName]; - let scope = desc.scope || 'model'; - let parts; + if (scope === 'controller') { + parts = []; + } - if (scope === 'controller') { - parts = []; - } + let urlKey = desc.as || this.serializeQueryParamKey(propName); + let defaultValue = get(controller, propName); - let urlKey = desc.as || this.serializeQueryParamKey(propName); - let defaultValue = get(controller, propName); + if (Array.isArray(defaultValue)) { + defaultValue = emberA(defaultValue.slice()); + } - if (Array.isArray(defaultValue)) { - defaultValue = emberA(defaultValue.slice()); + let type = desc.type || typeOf(defaultValue); + + let defaultValueSerialized = this.serializeQueryParam(defaultValue, urlKey, type); + let scopedPropertyName = `${controllerName}:${propName}`; + let qp = { + undecoratedDefaultValue: get(controller, propName), + defaultValue, + serializedDefaultValue: defaultValueSerialized, + serializedValue: defaultValueSerialized, + + type, + urlKey, + prop: propName, + scopedPropertyName, + controllerName, + route: this, + parts, // provided later when stashNames is called if 'model' scope + values: null, // provided later when setup is called. no idea why. + scope, + }; + + map[propName] = map[urlKey] = map[scopedPropertyName] = qp; + qps.push(qp); + propertyNames.push(propName); } - let type = desc.type || typeOf(defaultValue); - - let defaultValueSerialized = this.serializeQueryParam(defaultValue, urlKey, type); - let scopedPropertyName = `${controllerName}:${propName}`; - let qp = { - undecoratedDefaultValue: get(controller, propName), - defaultValue, - serializedDefaultValue: defaultValueSerialized, - serializedValue: defaultValueSerialized, - - type, - urlKey, - prop: propName, - scopedPropertyName, - controllerName, - route: this, - parts, // provided later when stashNames is called if 'model' scope - values: null, // provided later when setup is called. no idea why. - scope, + return { + qps, + map, + propertyNames, + states: { + /* + Called when a query parameter changes in the URL, this route cares + about that query parameter, but the route is not currently + in the active route hierarchy. + */ + inactive: (prop: string, value: unknown) => { + let qp = map[prop]; + this._qpChanged(prop, value, qp); + }, + /* + Called when a query parameter changes in the URL, this route cares + about that query parameter, and the route is currently + in the active route hierarchy. + */ + active: (prop: string, value: unknown) => { + let qp = map[prop]; + this._qpChanged(prop, value, qp); + return this._activeQPChanged(qp, value); + }, + /* + Called when a value of a query parameter this route handles changes in a controller + and the route is currently in the active route hierarchy. + */ + allowOverrides: (prop: string, value: unknown) => { + let qp = map[prop]; + this._qpChanged(prop, value, qp); + return this._updatingQPChanged(qp); + }, + }, }; + }, - map[propName] = map[urlKey] = map[scopedPropertyName] = qp; - qps.push(qp); - propertyNames.push(propName); - } - - return { - qps, - map, - propertyNames, - states: { - /* - Called when a query parameter changes in the URL, this route cares - about that query parameter, but the route is not currently - in the active route hierarchy. - */ - inactive: (prop: string, value: unknown) => { - let qp = map[prop]; - this._qpChanged(prop, value, qp); - }, - /* - Called when a query parameter changes in the URL, this route cares - about that query parameter, and the route is currently - in the active route hierarchy. - */ - active: (prop: string, value: unknown) => { - let qp = map[prop]; - this._qpChanged(prop, value, qp); - return this._activeQPChanged(qp, value); - }, - /* - Called when a value of a query parameter this route handles changes in a controller - and the route is currently in the active route hierarchy. - */ - allowOverrides: (prop: string, value: unknown) => { - let qp = map[prop]; - this._qpChanged(prop, value, qp); - return this._updatingQPChanged(qp); - }, - }, - }; + set(key, value) { + defineProperty(this, key, null, value); + }, }), /** diff --git a/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/observable_test.js b/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/observable_test.js index 220e4c3dce2..3adc1c920eb 100644 --- a/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/observable_test.js +++ b/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/observable_test.js @@ -48,7 +48,7 @@ moduleFor( object = ObservableObject.extend(Observable, { computed: computed(function() { return 'value'; - }).volatile(), + }), method() { return 'value'; }, @@ -97,7 +97,7 @@ moduleFor( objectA = ObservableObject.extend({ computed: computed(function() { return 'value'; - }).volatile(), + }), method() { return 'value'; }, @@ -164,7 +164,7 @@ moduleFor( bar: ObservableObject.extend({ baz: computed(function() { return 'blargh'; - }).volatile(), + }), }).create(), }); @@ -202,7 +202,7 @@ moduleFor( this._computed = value; return this._computed; }, - }).volatile(), + }), method(key, value) { if (value !== undefined) { @@ -284,99 +284,95 @@ moduleFor( beforeEach() { lookup = context.lookup = {}; - object = ObservableObject.extend({ - computed: computed({ - get() { - this.computedCalls.push('getter-called'); - return 'computed'; - }, - set(key, value) { - this.computedCalls.push(value); - }, - }).volatile(), + expectDeprecation(() => { + object = ObservableObject.extend({ + computed: computed({ + get() { + this.computedCalls.push('getter-called'); + return 'computed'; + }, + set(key, value) { + this.computedCalls.push(value); + }, + }).volatile(), - computedCached: computed({ - get() { - this.computedCachedCalls.push('getter-called'); - return 'computedCached'; - }, - set: function(key, value) { - this.computedCachedCalls.push(value); - }, - }), + computedCached: computed({ + get() { + this.computedCachedCalls.push('getter-called'); + return 'computedCached'; + }, + set: function(key, value) { + this.computedCachedCalls.push(value); + }, + }), - dependent: computed({ - get() { - this.dependentCalls.push('getter-called'); - return 'dependent'; - }, - set(key, value) { - this.dependentCalls.push(value); - }, - }) - .property('changer') - .volatile(), - dependentFront: computed('changer', { - get() { - this.dependentFrontCalls.push('getter-called'); - return 'dependentFront'; - }, - set(key, value) { - this.dependentFrontCalls.push(value); - }, - }).volatile(), - dependentCached: computed({ - get() { - this.dependentCachedCalls.push('getter-called!'); - return 'dependentCached'; - }, - set(key, value) { - this.dependentCachedCalls.push(value); - }, - }).property('changer'), + dependent: computed('changer', { + get() { + this.dependentCalls.push('getter-called'); + return 'dependent'; + }, + set(key, value) { + this.dependentCalls.push(value); + }, + }).volatile(), + dependentFront: computed('changer', { + get() { + this.dependentFrontCalls.push('getter-called'); + return 'dependentFront'; + }, + set(key, value) { + this.dependentFrontCalls.push(value); + }, + }).volatile(), + dependentCached: computed('changer', { + get() { + this.dependentCachedCalls.push('getter-called!'); + return 'dependentCached'; + }, + set(key, value) { + this.dependentCachedCalls.push(value); + }, + }), - inc: computed('changer', function() { - return this.incCallCount++; - }), + inc: computed('changer', function() { + return this.incCallCount++; + }), - nestedInc: computed(function() { - get(this, 'inc'); - return this.nestedIncCallCount++; - }).property('inc'), + nestedInc: computed('inc', function() { + get(this, 'inc'); + return this.nestedIncCallCount++; + }), - isOn: computed({ - get() { - return this.get('state') === 'on'; - }, - set() { - this.set('state', 'on'); - return this.get('state') === 'on'; - }, - }) - .property('state') - .volatile(), + isOn: computed('state', { + get() { + return this.get('state') === 'on'; + }, + set() { + this.set('state', 'on'); + return this.get('state') === 'on'; + }, + }).volatile(), - isOff: computed({ - get() { - return this.get('state') === 'off'; - }, - set() { - this.set('state', 'off'); - return this.get('state') === 'off'; - }, - }) - .property('state') - .volatile(), - }).create({ - computedCalls: [], - computedCachedCalls: [], - changer: 'foo', - dependentCalls: [], - dependentFrontCalls: [], - dependentCachedCalls: [], - incCallCount: 0, - nestedIncCallCount: 0, - state: 'on', + isOff: computed('state', { + get() { + return this.get('state') === 'off'; + }, + set() { + this.set('state', 'off'); + return this.get('state') === 'off'; + }, + }).volatile(), + }).create({ + computedCalls: [], + computedCachedCalls: [], + changer: 'foo', + dependentCalls: [], + dependentFrontCalls: [], + dependentCachedCalls: [], + incCallCount: 0, + nestedIncCallCount: 0, + state: 'on', + }); }); } afterEach() { @@ -542,9 +538,9 @@ moduleFor( ['@test dependent keys should be able to be specified as property paths'](assert) { var depObj = ObservableObject.extend({ - menuPrice: computed(function() { + menuPrice: computed('menu.price', function() { return this.get('menu.price'); - }).property('menu.price'), + }), }).create({ menu: ObservableObject.create({ price: 5, diff --git a/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js b/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js index fa70f6cb17e..d73e9faa7bf 100644 --- a/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js +++ b/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js @@ -112,31 +112,33 @@ moduleFor( ['@test should invalidate function property cache when notifyPropertyChange is called']( assert ) { - let a = ObservableObject.extend({ - b: computed({ - get() { - return this._b; - }, - set(key, value) { - this._b = value; - return this; - }, - }).volatile(), - }).create({ - _b: null, - }); - - a.set('b', 'foo'); - assert.equal(a.get('b'), 'foo', 'should have set the correct value for property b'); - - a._b = 'bar'; - a.notifyPropertyChange('b'); - a.set('b', 'foo'); - assert.equal( - a.get('b'), - 'foo', - 'should have invalidated the cache so that the newly set value is actually set' - ); + expectDeprecation(() => { + let a = ObservableObject.extend({ + b: computed({ + get() { + return this._b; + }, + set(key, value) { + this._b = value; + return this; + }, + }).volatile(), + }).create({ + _b: null, + }); + + a.set('b', 'foo'); + assert.equal(a.get('b'), 'foo', 'should have set the correct value for property b'); + + a._b = 'bar'; + a.notifyPropertyChange('b'); + a.set('b', 'foo'); + assert.equal( + a.get('b'), + 'foo', + 'should have invalidated the cache so that the newly set value is actually set' + ); + }, /Setting a computed property as volatile has been deprecated/); } } ); diff --git a/packages/@ember/-internals/runtime/tests/system/array_proxy/arranged_content_test.js b/packages/@ember/-internals/runtime/tests/system/array_proxy/arranged_content_test.js index 07048f0ae3b..c5a91b7de73 100644 --- a/packages/@ember/-internals/runtime/tests/system/array_proxy/arranged_content_test.js +++ b/packages/@ember/-internals/runtime/tests/system/array_proxy/arranged_content_test.js @@ -146,7 +146,7 @@ moduleFor( beforeEach() { run(function() { array = ArrayProxy.extend({ - arrangedContent: computed(function() { + arrangedContent: computed('content.[]', function() { let content = this.get('content'); return ( content && @@ -162,7 +162,7 @@ moduleFor( }) ) ); - }).property('content.[]'), + }), objectAtContent(idx) { let obj = objectAt(this.get('arrangedContent'), idx); diff --git a/packages/@ember/-internals/runtime/tests/system/object/computed_test.js b/packages/@ember/-internals/runtime/tests/system/object/computed_test.js index 9ea2737f735..d48ae6f9d7d 100644 --- a/packages/@ember/-internals/runtime/tests/system/object/computed_test.js +++ b/packages/@ember/-internals/runtime/tests/system/object/computed_test.js @@ -73,10 +73,10 @@ moduleFor( count: 0, - foo: computed(function() { + foo: computed('bar.baz', function() { set(this, 'count', get(this, 'count') + 1); return get(get(this, 'bar'), 'baz') + ' ' + get(this, 'count'); - }).property('bar.baz'), + }), }); let Subclass = MyClass.extend({ @@ -109,10 +109,10 @@ moduleFor( count: 0, - foo: computed(function() { + foo: computed('bar.baz', function() { set(this, 'count', get(this, 'count') + 1); return get(get(this, 'bar'), 'baz') + ' ' + get(this, 'count'); - }).property('bar.baz'), + }), }); let Subclass = MyClass.extend({ @@ -123,10 +123,10 @@ moduleFor( count: 0, - foo: computed(function() { + foo: computed('bar2.baz', function() { set(this, 'count', get(this, 'count') + 1); return get(get(this, 'bar2'), 'baz') + ' ' + get(this, 'count'); - }).property('bar2.baz'), + }), }); let obj2 = Subclass.create(); @@ -152,7 +152,7 @@ moduleFor( ); let ClassWithNoMetadata = EmberObject.extend({ - computedProperty: computed(function() {}).volatile(), + computedProperty: computed(function() {}), staticProperty: 12, }); @@ -356,5 +356,23 @@ moduleFor( assert.equal(obj1.get('name'), '1'); assert.equal(obj2.get('name'), '2'); } + + ['@test can declare dependent keys with .property()'](assert) { + expectDeprecation(() => { + let Obj = EmberObject.extend({ + foo: computed(function() { + return this.bar; + }).property('bar'), + }); + + let obj = Obj.create({ bar: 1 }); + + assert.equal(obj.get('foo'), 1); + + obj.set('bar', 2); + + assert.equal(obj.get('foo'), 2); + }, /Setting dependency keys using the `.property\(\)` modifier has been deprecated/); + } } ); diff --git a/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js b/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js index 3a4c5a53ed4..0a73fc39a1b 100644 --- a/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js +++ b/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js @@ -180,7 +180,7 @@ moduleFor( let last; let Proxy = ObjectProxy.extend({ - fullName: computed(function() { + fullName: computed('firstName', 'lastName', function() { let firstName = this.get('firstName'); let lastName = this.get('lastName'); @@ -188,7 +188,7 @@ moduleFor( return firstName + ' ' + lastName; } return firstName || lastName; - }).property('firstName', 'lastName'), + }), }); let proxy = Proxy.create(); diff --git a/packages/@ember/object/lib/computed/reduce_computed_macros.js b/packages/@ember/object/lib/computed/reduce_computed_macros.js index 8f650e13f52..c039665ade1 100644 --- a/packages/@ember/object/lib/computed/reduce_computed_macros.js +++ b/packages/@ember/object/lib/computed/reduce_computed_macros.js @@ -1,8 +1,15 @@ /** @module @ember/object */ +import { DEBUG } from '@glimmer/env'; import { assert } from '@ember/debug'; -import { get, ComputedProperty, addObserver, removeObserver } from '@ember/-internals/metal'; +import { + get, + computed, + ComputedProperty, + addObserver, + removeObserver, +} from '@ember/-internals/metal'; import { compare, isArray, A as emberA, uniqBy as uniqByArray } from '@ember/-internals/runtime'; function reduceMacro(dependentKey, callback, initialValue, name) { @@ -25,7 +32,7 @@ function reduceMacro(dependentKey, callback, initialValue, name) { return cp; } -function arrayMacro(dependentKey, callback) { +function arrayMacro(dependentKey, additionalDependentKeys, callback) { // This is a bit ugly let propertyName; if (/@each/.test(dependentKey)) { @@ -35,21 +42,14 @@ function arrayMacro(dependentKey, callback) { dependentKey += '.[]'; } - let cp = new ComputedProperty( - function() { - let value = get(this, propertyName); - if (isArray(value)) { - return emberA(callback.call(this, value)); - } else { - return emberA(); - } - }, - { readOnly: true } - ); - - cp.property(dependentKey); // this forces to expand properties GH #15855 - - return cp; + return computed(dependentKey, ...additionalDependentKeys, function() { + let value = get(this, propertyName); + if (isArray(value)) { + return emberA(callback.call(this, value)); + } else { + return emberA(); + } + }).readOnly(); } function multiArrayMacro(_dependentKeys, callback, name) { @@ -217,12 +217,28 @@ export function min(dependentKey) { @for @ember/object/computed @static @param {String} dependentKey + @param {Array} additionalDependentKeys optional array of additional dependent keys @param {Function} callback @return {ComputedProperty} an array mapped via the callback @public */ -export function map(dependentKey, callback) { - return arrayMacro(dependentKey, function(value) { +export function map(dependentKey, additionalDependentKeys, callback) { + if (callback === undefined && typeof additionalDependentKeys === 'function') { + callback = additionalDependentKeys; + additionalDependentKeys = []; + } + + assert( + 'The final parameter provided to map must be a callback function', + typeof callback === 'function' + ); + + assert( + 'The second parameter provided to map must either be the callback or an array of additional dependent keys', + Array.isArray(additionalDependentKeys) + ); + + return arrayMacro(dependentKey, additionalDependentKeys, function(value) { return value.map(callback, this); }); } @@ -338,12 +354,28 @@ export function mapBy(dependentKey, propertyKey) { @for @ember/object/computed @static @param {String} dependentKey + @param {Array} additionalDependentKeys optional array of additional dependent keys @param {Function} callback @return {ComputedProperty} the filtered array @public */ -export function filter(dependentKey, callback) { - return arrayMacro(dependentKey, function(value) { +export function filter(dependentKey, additionalDependentKeys, callback) { + if (callback === undefined && typeof additionalDependentKeys === 'function') { + callback = additionalDependentKeys; + additionalDependentKeys = []; + } + + assert( + 'The final parameter provided to filter must be a callback function', + typeof callback === 'function' + ); + + assert( + 'The second parameter provided to filter must either be the callback or an array of additional dependent keys', + Array.isArray(additionalDependentKeys) + ); + + return arrayMacro(dependentKey, additionalDependentKeys, function(value) { return value.filter(callback, this); }); } @@ -782,28 +814,51 @@ export function collect(...dependentKeys) { @for @ember/object/computed @static @param {String} itemsKey + @param {Array} additionalDependentKeys optional array of additional dependent keys @param {String or Function} sortDefinition a dependent key to an array of sort properties (add `:desc` to the arrays sort properties to sort descending) or a function to use when sorting @return {ComputedProperty} computes a new sorted array based on the sort property array or callback function @public */ -export function sort(itemsKey, sortDefinition) { - assert( - '`computed.sort` requires two arguments: an array key to sort and ' + - 'either a sort properties key or sort function', - arguments.length === 2 - ); +export function sort(itemsKey, additionalDependentKeys, sortDefinition) { + if (DEBUG) { + let argumentsValid = false; + + if (arguments.length === 2) { + argumentsValid = + typeof itemsKey === 'string' && + (typeof additionalDependentKeys === 'string' || + typeof additionalDependentKeys === 'function'); + } + + if (arguments.length === 3) { + argumentsValid = + typeof itemsKey === 'string' && + Array.isArray(additionalDependentKeys) && + typeof sortDefinition === 'function'; + } + + assert( + '`computed.sort` can either be used with an array of sort properties or with a sort function. If used with an array of sort properties, it must receive exactly two arguments: the key of the array to sort, and the key of the array of sort properties. If used with a sort function, it may recieve up to three arguments: the key of the array to sort, an optional additional array of dependent keys for the computed property, and the sort function.', + argumentsValid + ); + } + + if (sortDefinition === undefined && !Array.isArray(additionalDependentKeys)) { + sortDefinition = additionalDependentKeys; + additionalDependentKeys = []; + } if (typeof sortDefinition === 'function') { - return customSort(itemsKey, sortDefinition); + return customSort(itemsKey, additionalDependentKeys, sortDefinition); } else { return propertySort(itemsKey, sortDefinition); } } -function customSort(itemsKey, comparator) { - return arrayMacro(itemsKey, function(value) { +function customSort(itemsKey, additionalDependentKeys, comparator) { + return arrayMacro(itemsKey, additionalDependentKeys, function(value) { return value.slice().sort((x, y) => comparator.call(this, x, y)); }); } diff --git a/packages/@ember/object/tests/computed/reduce_computed_macros_test.js b/packages/@ember/object/tests/computed/reduce_computed_macros_test.js index d9b9878cee2..ec5fef112ad 100644 --- a/packages/@ember/object/tests/computed/reduce_computed_macros_test.js +++ b/packages/@ember/object/tests/computed/reduce_computed_macros_test.js @@ -172,6 +172,48 @@ moduleFor( 'properties unshifted in sequence are mapped correctly' ); } + + ['@test it updates if additional dependent keys are modified'](assert) { + obj = EmberObject.extend({ + mapped: map('array', ['key'], function(item) { + return item[this.key]; + }), + }).create({ + key: 'name', + array: emberA([{ name: 'Cercei', house: 'Lannister' }]), + }); + + assert.deepEqual( + obj.get('mapped'), + ['Cercei'], + 'precond - mapped array is initially correct' + ); + + obj.set('key', 'house'); + assert.deepEqual( + obj.get('mapped'), + ['Lannister'], + 'mapped prop updates correctly when additional dependency is updated' + ); + } + + ['@test it throws on bad inputs']() { + expectAssertion(() => { + map('items.@each.{prop}', 'foo'); + }, /The final parameter provided to map must be a callback function/); + + expectAssertion(() => { + map('items.@each.{prop}', 'foo', function() {}); + }, /The second parameter provided to map must either be the callback or an array of additional dependent keys/); + + expectAssertion(() => { + map('items.@each.{prop}', function() {}, ['foo']); + }, /The final parameter provided to map must be a callback function/); + + expectAssertion(() => { + map('items.@each.{prop}', ['foo']); + }, /The final parameter provided to map must be a callback function/); + } } ); @@ -401,6 +443,48 @@ moduleFor( assert.deepEqual(obj.get('filtered'), []); } + + ['@test it updates if additional dependent keys are modified'](assert) { + obj = EmberObject.extend({ + filtered: filter('array', ['modulo'], function(item) { + return item % this.modulo === 0; + }), + }).create({ + modulo: 2, + array: emberA([1, 2, 3, 4, 5, 6, 7, 8]), + }); + + assert.deepEqual( + obj.get('filtered'), + [2, 4, 6, 8], + 'precond - filtered array is initially correct' + ); + + obj.set('modulo', 3); + assert.deepEqual( + obj.get('filtered'), + [3, 6], + 'filtered prop updates correctly when additional dependency is updated' + ); + } + + ['@test it throws on bad inputs']() { + expectAssertion(() => { + filter('items.@each.{prop}', 'foo'); + }, /The final parameter provided to filter must be a callback function/); + + expectAssertion(() => { + filter('items.@each.{prop}', 'foo', function() {}); + }, /The second parameter provided to filter must either be the callback or an array of additional dependent keys/); + + expectAssertion(() => { + filter('items.@each.{prop}', function() {}, ['foo']); + }, /The final parameter provided to filter must be a callback function/); + + expectAssertion(() => { + filter('items.@each.{prop}', ['foo']); + }, /The final parameter provided to filter must be a callback function/); + } } ); @@ -1726,6 +1810,62 @@ moduleFor( assert.expect(0); } } + + ['@test sort updates if additional dependent keys are present'](assert) { + obj = EmberObject.extend({ + sortedItems: sort('items', ['sortFunction'], function() { + return this.sortFunction(...arguments); + }), + }).create({ + sortFunction: sortByLnameFname, + items: emberA([ + { fname: 'Jaime', lname: 'Lannister', age: 34 }, + { fname: 'Cersei', lname: 'Lannister', age: 34 }, + { fname: 'Robb', lname: 'Stark', age: 16 }, + { fname: 'Bran', lname: 'Stark', age: 8 }, + ]), + }); + + assert.deepEqual( + obj.get('sortedItems').mapBy('fname'), + ['Cersei', 'Jaime', 'Bran', 'Robb'], + 'array is initially sorted' + ); + + obj.set('sortFunction', (a, b) => { + if (a.age > b.age) { + return -1; + } else if (a.age < b.age) { + return 1; + } + + return 0; + }); + + assert.deepEqual( + obj.get('sortedItems').mapBy('fname'), + ['Jaime', 'Cersei', 'Robb', 'Bran'], + 'array is updated when dependent key changes' + ); + } + + ['@test it throws on bad inputs']() { + expectAssertion(() => { + sort('foo', 'bar', 'baz'); + }, /`computed.sort` can either be used with an array of sort properties or with a sort function/); + + expectAssertion(() => { + sort('foo', ['bar'], 'baz'); + }, /`computed.sort` can either be used with an array of sort properties or with a sort function/); + + expectAssertion(() => { + sort('foo', 'bar', function() {}); + }, /`computed.sort` can either be used with an array of sort properties or with a sort function/); + + expectAssertion(() => { + sort('foo', ['bar'], function() {}, 'baz'); + }, /`computed.sort` can either be used with an array of sort properties or with a sort function/); + } } ); From 95973cf35a59fae073889d9a89a16694cb07803f Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Fri, 11 Jan 2019 15:46:58 -0800 Subject: [PATCH 2/2] review feedback --- .../@ember/-internals/metal/lib/computed.ts | 6 +- .../-internals/metal/tests/computed_test.js | 72 +++--- .../metal/tests/mixin/computed_test.js | 104 ++++---- .../-internals/routing/lib/system/route.ts | 226 +++++++++--------- .../routing/tests/system/route_test.js | 4 +- .../mixins/observable/propertyChanges_test.js | 28 ++- .../tests/system/object/computed_test.js | 14 +- .../lib/computed/reduce_computed_macros.js | 163 +++++++++++-- 8 files changed, 368 insertions(+), 249 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts index 6142029941a..9b9b50dc03a 100644 --- a/packages/@ember/-internals/metal/lib/computed.ts +++ b/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'; @@ -476,7 +476,9 @@ 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', diff --git a/packages/@ember/-internals/metal/tests/computed_test.js b/packages/@ember/-internals/metal/tests/computed_test.js index f37e707710f..d524a067d5b 100644 --- a/packages/@ember/-internals/metal/tests/computed_test.js +++ b/packages/@ember/-internals/metal/tests/computed_test.js @@ -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) { @@ -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) { @@ -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'); } } ); diff --git a/packages/@ember/-internals/metal/tests/mixin/computed_test.js b/packages/@ember/-internals/metal/tests/mixin/computed_test.js index 1a9a07afd6c..071464c5d86 100644 --- a/packages/@ember/-internals/metal/tests/mixin/computed_test.js +++ b/packages/@ember/-internals/metal/tests/mixin/computed_test.js @@ -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'); } } ); diff --git a/packages/@ember/-internals/routing/lib/system/route.ts b/packages/@ember/-internals/routing/lib/system/route.ts index babf671acce..392247e6836 100644 --- a/packages/@ember/-internals/routing/lib/system/route.ts +++ b/packages/@ember/-internals/routing/lib/system/route.ts @@ -2185,134 +2185,128 @@ Route.reopen(ActionHandler, Evented, { @property _qp */ - _qp: computed({ - get(this: Route) { - let combinedQueryParameterConfiguration; + _qp: computed(function(this: Route) { + let combinedQueryParameterConfiguration; - let controllerName = this.controllerName || this.routeName; - let owner = getOwner(this); - let controller = owner.lookup(`controller:${controllerName}`); - let queryParameterConfiguraton = get(this, 'queryParams'); - let hasRouterDefinedQueryParams = Object.keys(queryParameterConfiguraton).length > 0; - - if (controller) { - // the developer has authored a controller class in their application for - // this route find its query params and normalize their object shape them - // merge in the query params for the route. As a mergedProperty, - // Route#queryParams is always at least `{}` - - let controllerDefinedQueryParameterConfiguration = get(controller, 'queryParams') || {}; - let normalizedControllerQueryParameterConfiguration = normalizeControllerQueryParams( - controllerDefinedQueryParameterConfiguration - ); - combinedQueryParameterConfiguration = mergeEachQueryParams( - normalizedControllerQueryParameterConfiguration, - queryParameterConfiguraton - ); - } else if (hasRouterDefinedQueryParams) { - // the developer has not defined a controller but *has* supplied route query params. - // Generate a class for them so we can later insert default values - controller = generateController(owner, controllerName); - combinedQueryParameterConfiguration = queryParameterConfiguraton; - } - - let qps = []; - let map = {}; - let propertyNames = []; - - for (let propName in combinedQueryParameterConfiguration) { - if (!combinedQueryParameterConfiguration.hasOwnProperty(propName)) { - continue; - } + let controllerName = this.controllerName || this.routeName; + let owner = getOwner(this); + let controller = owner.lookup(`controller:${controllerName}`); + let queryParameterConfiguraton = get(this, 'queryParams'); + let hasRouterDefinedQueryParams = Object.keys(queryParameterConfiguraton).length > 0; + + if (controller) { + // the developer has authored a controller class in their application for + // this route find its query params and normalize their object shape them + // merge in the query params for the route. As a mergedProperty, + // Route#queryParams is always at least `{}` + + let controllerDefinedQueryParameterConfiguration = get(controller, 'queryParams') || {}; + let normalizedControllerQueryParameterConfiguration = normalizeControllerQueryParams( + controllerDefinedQueryParameterConfiguration + ); + combinedQueryParameterConfiguration = mergeEachQueryParams( + normalizedControllerQueryParameterConfiguration, + queryParameterConfiguraton + ); + } else if (hasRouterDefinedQueryParams) { + // the developer has not defined a controller but *has* supplied route query params. + // Generate a class for them so we can later insert default values + controller = generateController(owner, controllerName); + combinedQueryParameterConfiguration = queryParameterConfiguraton; + } - // to support the dubious feature of using unknownProperty - // on queryParams configuration - if (propName === 'unknownProperty' || propName === '_super') { - // possible todo: issue deprecation warning? - continue; - } + let qps = []; + let map = {}; + let propertyNames = []; - let desc = combinedQueryParameterConfiguration[propName]; - let scope = desc.scope || 'model'; - let parts; + for (let propName in combinedQueryParameterConfiguration) { + if (!combinedQueryParameterConfiguration.hasOwnProperty(propName)) { + continue; + } - if (scope === 'controller') { - parts = []; - } + // to support the dubious feature of using unknownProperty + // on queryParams configuration + if (propName === 'unknownProperty' || propName === '_super') { + // possible todo: issue deprecation warning? + continue; + } - let urlKey = desc.as || this.serializeQueryParamKey(propName); - let defaultValue = get(controller, propName); + let desc = combinedQueryParameterConfiguration[propName]; + let scope = desc.scope || 'model'; + let parts; - if (Array.isArray(defaultValue)) { - defaultValue = emberA(defaultValue.slice()); - } + if (scope === 'controller') { + parts = []; + } - let type = desc.type || typeOf(defaultValue); - - let defaultValueSerialized = this.serializeQueryParam(defaultValue, urlKey, type); - let scopedPropertyName = `${controllerName}:${propName}`; - let qp = { - undecoratedDefaultValue: get(controller, propName), - defaultValue, - serializedDefaultValue: defaultValueSerialized, - serializedValue: defaultValueSerialized, - - type, - urlKey, - prop: propName, - scopedPropertyName, - controllerName, - route: this, - parts, // provided later when stashNames is called if 'model' scope - values: null, // provided later when setup is called. no idea why. - scope, - }; + let urlKey = desc.as || this.serializeQueryParamKey(propName); + let defaultValue = get(controller, propName); - map[propName] = map[urlKey] = map[scopedPropertyName] = qp; - qps.push(qp); - propertyNames.push(propName); + if (Array.isArray(defaultValue)) { + defaultValue = emberA(defaultValue.slice()); } - return { - qps, - map, - propertyNames, - states: { - /* - Called when a query parameter changes in the URL, this route cares - about that query parameter, but the route is not currently - in the active route hierarchy. - */ - inactive: (prop: string, value: unknown) => { - let qp = map[prop]; - this._qpChanged(prop, value, qp); - }, - /* - Called when a query parameter changes in the URL, this route cares - about that query parameter, and the route is currently - in the active route hierarchy. - */ - active: (prop: string, value: unknown) => { - let qp = map[prop]; - this._qpChanged(prop, value, qp); - return this._activeQPChanged(qp, value); - }, - /* - Called when a value of a query parameter this route handles changes in a controller - and the route is currently in the active route hierarchy. - */ - allowOverrides: (prop: string, value: unknown) => { - let qp = map[prop]; - this._qpChanged(prop, value, qp); - return this._updatingQPChanged(qp); - }, - }, + let type = desc.type || typeOf(defaultValue); + + let defaultValueSerialized = this.serializeQueryParam(defaultValue, urlKey, type); + let scopedPropertyName = `${controllerName}:${propName}`; + let qp = { + undecoratedDefaultValue: get(controller, propName), + defaultValue, + serializedDefaultValue: defaultValueSerialized, + serializedValue: defaultValueSerialized, + + type, + urlKey, + prop: propName, + scopedPropertyName, + controllerName, + route: this, + parts, // provided later when stashNames is called if 'model' scope + values: null, // provided later when setup is called. no idea why. + scope, }; - }, - set(key, value) { - defineProperty(this, key, null, value); - }, + map[propName] = map[urlKey] = map[scopedPropertyName] = qp; + qps.push(qp); + propertyNames.push(propName); + } + + return { + qps, + map, + propertyNames, + states: { + /* + Called when a query parameter changes in the URL, this route cares + about that query parameter, but the route is not currently + in the active route hierarchy. + */ + inactive: (prop: string, value: unknown) => { + let qp = map[prop]; + this._qpChanged(prop, value, qp); + }, + /* + Called when a query parameter changes in the URL, this route cares + about that query parameter, and the route is currently + in the active route hierarchy. + */ + active: (prop: string, value: unknown) => { + let qp = map[prop]; + this._qpChanged(prop, value, qp); + return this._activeQPChanged(qp, value); + }, + /* + Called when a value of a query parameter this route handles changes in a controller + and the route is currently in the active route hierarchy. + */ + allowOverrides: (prop: string, value: unknown) => { + let qp = map[prop]; + this._qpChanged(prop, value, qp); + return this._updatingQPChanged(qp); + }, + }, + }; }), /** diff --git a/packages/@ember/-internals/routing/tests/system/route_test.js b/packages/@ember/-internals/routing/tests/system/route_test.js index 90d5a29cd4a..870db3fbfea 100644 --- a/packages/@ember/-internals/routing/tests/system/route_test.js +++ b/packages/@ember/-internals/routing/tests/system/route_test.js @@ -3,6 +3,7 @@ import { runDestroy, buildOwner, moduleFor, AbstractTestCase } from 'internal-te import Service, { inject as injectService } from '@ember/service'; import { Object as EmberObject } from '@ember/-internals/runtime'; import EmberRoute from '../../lib/system/route'; +import { defineProperty } from '../../../metal'; let route, routeOne, routeTwo, lookupHash; @@ -53,7 +54,8 @@ moduleFor( let owner = buildOwner(ownerOptions); setOwner(route, owner); - route.set('_qp', null); + // Override the computed property by redefining it + defineProperty(route, '_qp', null, null); assert.equal(route.model({ post_id: 1 }), post); assert.equal(route.findModel('post', 1), post, '#findModel returns the correct post'); diff --git a/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js b/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js index d73e9faa7bf..a672cdd8754 100644 --- a/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js +++ b/packages/@ember/-internals/runtime/tests/legacy_1x/mixins/observable/propertyChanges_test.js @@ -112,8 +112,10 @@ moduleFor( ['@test should invalidate function property cache when notifyPropertyChange is called']( assert ) { + let a; + expectDeprecation(() => { - let a = ObservableObject.extend({ + a = ObservableObject.extend({ b: computed({ get() { return this._b; @@ -126,19 +128,19 @@ moduleFor( }).create({ _b: null, }); - - a.set('b', 'foo'); - assert.equal(a.get('b'), 'foo', 'should have set the correct value for property b'); - - a._b = 'bar'; - a.notifyPropertyChange('b'); - a.set('b', 'foo'); - assert.equal( - a.get('b'), - 'foo', - 'should have invalidated the cache so that the newly set value is actually set' - ); }, /Setting a computed property as volatile has been deprecated/); + + a.set('b', 'foo'); + assert.equal(a.get('b'), 'foo', 'should have set the correct value for property b'); + + a._b = 'bar'; + a.notifyPropertyChange('b'); + a.set('b', 'foo'); + assert.equal( + a.get('b'), + 'foo', + 'should have invalidated the cache so that the newly set value is actually set' + ); } } ); diff --git a/packages/@ember/-internals/runtime/tests/system/object/computed_test.js b/packages/@ember/-internals/runtime/tests/system/object/computed_test.js index d48ae6f9d7d..7bbf03b4bfd 100644 --- a/packages/@ember/-internals/runtime/tests/system/object/computed_test.js +++ b/packages/@ember/-internals/runtime/tests/system/object/computed_test.js @@ -358,21 +358,23 @@ moduleFor( } ['@test can declare dependent keys with .property()'](assert) { + let Obj; + expectDeprecation(() => { - let Obj = EmberObject.extend({ + Obj = EmberObject.extend({ foo: computed(function() { return this.bar; }).property('bar'), }); + }, /Setting dependency keys using the `.property\(\)` modifier has been deprecated/); - let obj = Obj.create({ bar: 1 }); + let obj = Obj.create({ bar: 1 }); - assert.equal(obj.get('foo'), 1); + assert.equal(obj.get('foo'), 1); - obj.set('bar', 2); + obj.set('bar', 2); - assert.equal(obj.get('foo'), 2); - }, /Setting dependency keys using the `.property\(\)` modifier has been deprecated/); + assert.equal(obj.get('foo'), 2); } } ); diff --git a/packages/@ember/object/lib/computed/reduce_computed_macros.js b/packages/@ember/object/lib/computed/reduce_computed_macros.js index c039665ade1..4f5a864bd40 100644 --- a/packages/@ember/object/lib/computed/reduce_computed_macros.js +++ b/packages/@ember/object/lib/computed/reduce_computed_macros.js @@ -213,11 +213,39 @@ export function min(dependentKey) { hamster.get('excitingChores'); // ['CLEAN!', 'WRITE MORE UNIT TESTS!'] ``` + You can optionally pass an array of additional dependent keys as the second + parameter to the macro, if your map function relies on any external values: + + ```javascript + import { map } from '@ember/object/computed'; + import EmberObject from '@ember/object'; + + let Hamster = EmberObject.extend({ + excitingChores: map('chores', ['shouldUpperCase'], function(chore, index) { + if (this.shouldUpperCase) { + return chore.toUpperCase() + '!'; + } else { + return chore + '!'; + } + }) + }); + + let hamster = Hamster.create({ + shouldUpperCase: false, + + chores: ['clean', 'write more unit tests'] + }); + + hamster.get('excitingChores'); // ['clean!', 'write more unit tests!'] + hamster.set('shouldUpperCase', true); + hamster.get('excitingChores'); // ['CLEAN!', 'WRITE MORE UNIT TESTS!'] + ``` + @method map @for @ember/object/computed @static @param {String} dependentKey - @param {Array} additionalDependentKeys optional array of additional dependent keys + @param {Array} [additionalDependentKeys] optional array of additional dependent keys @param {Function} callback @return {ComputedProperty} an array mapped via the callback @public @@ -349,12 +377,38 @@ export function mapBy(dependentKey, propertyKey) { hamster.get('remainingChores'); // [] ``` + Finally, you can optionally pass an array of additional dependent keys as the + second parameter to the macro, if your filter function relies on any external + values: + + ```javascript + import { filter } from '@ember/object/computed'; + import EmberObject from '@ember/object'; + + let Hamster = EmberObject.extend({ + remainingChores: filter('chores', ['doneKey'], function(chore, index, array) { + return !chore[this.doneKey]; + }) + }); + + let hamster = Hamster.create({ + doneKey: 'finished' + + chores: [ + { name: 'cook', finished: true }, + { name: 'clean', finished: true }, + { name: 'write more unit tests', finished: false } + ] + }); + + hamster.get('remainingChores'); // [{name: 'write more unit tests', finished: false}] + ``` @method filter @for @ember/object/computed @static @param {String} dependentKey - @param {Array} additionalDependentKeys optional array of additional dependent keys + @param {Array} [additionalDependentKeys] optional array of additional dependent keys @param {Function} callback @return {ComputedProperty} the filtered array @public @@ -754,9 +808,13 @@ export function collect(...dependentKeys) { /** A computed property which returns a new array with all the properties from the first dependent array sorted based on a property - or sort function. + or sort function. The sort macro can be used in two different ways: + + 1. By providing a sort callback function + 2. By providing an array of keys to sort the array - The callback method you provide should have the following signature: + In the first form, the callback method you provide should have the following + signature: ```javascript function(itemA, itemB); @@ -765,12 +823,14 @@ export function collect(...dependentKeys) { - `itemA` the first item to compare. - `itemB` the second item to compare. - This function should return negative number (e.g. `-1`) when `itemA` should come before - `itemB`. It should return positive number (e.g. `1`) when `itemA` should come after - `itemB`. If the `itemA` and `itemB` are equal this function should return `0`. + This function should return negative number (e.g. `-1`) when `itemA` should + come before `itemB`. It should return positive number (e.g. `1`) when `itemA` + should come after `itemB`. If the `itemA` and `itemB` are equal this function + should return `0`. - Therefore, if this function is comparing some numeric values, simple `itemA - itemB` or - `itemA.get( 'foo' ) - itemB.get( 'foo' )` can be used instead of series of `if`. + Therefore, if this function is comparing some numeric values, simple `itemA - + itemB` or `itemA.get( 'foo' ) - itemB.get( 'foo' )` can be used instead of + series of `if`. Example @@ -779,14 +839,6 @@ export function collect(...dependentKeys) { import EmberObject from '@ember/object'; let ToDoList = EmberObject.extend({ - // using standard ascending sort - todosSorting: Object.freeze(['name']), - sortedTodos: sort('todos', 'todosSorting'), - - // using descending sort - todosSortingDesc: Object.freeze(['name:desc']), - sortedTodosDesc: sort('todos', 'todosSortingDesc'), - // using a custom sort function priorityTodos: sort('todos', function(a, b){ if (a.priority > b.priority) { @@ -799,22 +851,85 @@ export function collect(...dependentKeys) { }) }); - let todoList = ToDoList.create({todos: [ - { name: 'Unit Test', priority: 2 }, - { name: 'Documentation', priority: 3 }, - { name: 'Release', priority: 1 } - ]}); + let todoList = ToDoList.create({ + todos: [ + { name: 'Unit Test', priority: 2 }, + { name: 'Documentation', priority: 3 }, + { name: 'Release', priority: 1 } + ] + }); + + todoList.get('priorityTodos'); // [{ name:'Release', priority:1 }, { name:'Unit Test', priority:2 }, { name:'Documentation', priority:3 }] + ``` + + You can also optionally pass an array of additional dependent keys as the + second parameter, if your sort function is dependent on additional values that + could changes: + + ```js + import { sort } from '@ember/object/computed'; + import EmberObject from '@ember/object'; + + let ToDoList = EmberObject.extend({ + // using a custom sort function + sortedTodos: sort('todos', ['sortKey'] function(a, b){ + if (a[this.sortKey] > b[this.sortKey]) { + return 1; + } else if (a[this.sortKey] < b[this.sortKey]) { + return -1; + } + + return 0; + }) + }); + + let todoList = ToDoList.create({ + sortKey: 'priority', + + todos: [ + { name: 'Unit Test', priority: 2 }, + { name: 'Documentation', priority: 3 }, + { name: 'Release', priority: 1 } + ] + }); + + todoList.get('priorityTodos'); // [{ name:'Release', priority:1 }, { name:'Unit Test', priority:2 }, { name:'Documentation', priority:3 }] + ``` + + In the second form, you should provide the key of the array of sort values as + the second parameter: + + ```javascript + import { sort } from '@ember/object/computed'; + import EmberObject from '@ember/object'; + + let ToDoList = EmberObject.extend({ + // using standard ascending sort + todosSorting: Object.freeze(['name']), + sortedTodos: sort('todos', 'todosSorting'), + + // using descending sort + todosSortingDesc: Object.freeze(['name:desc']), + sortedTodosDesc: sort('todos', 'todosSortingDesc'), + }); + + let todoList = ToDoList.create({ + todos: [ + { name: 'Unit Test', priority: 2 }, + { name: 'Documentation', priority: 3 }, + { name: 'Release', priority: 1 } + ] + }); todoList.get('sortedTodos'); // [{ name:'Documentation', priority:3 }, { name:'Release', priority:1 }, { name:'Unit Test', priority:2 }] todoList.get('sortedTodosDesc'); // [{ name:'Unit Test', priority:2 }, { name:'Release', priority:1 }, { name:'Documentation', priority:3 }] - todoList.get('priorityTodos'); // [{ name:'Release', priority:1 }, { name:'Unit Test', priority:2 }, { name:'Documentation', priority:3 }] ``` @method sort @for @ember/object/computed @static @param {String} itemsKey - @param {Array} additionalDependentKeys optional array of additional dependent keys + @param {Array} [additionalDependentKeys] optional array of additional dependent keys @param {String or Function} sortDefinition a dependent key to an array of sort properties (add `:desc` to the arrays sort properties to sort descending) or a function to use when sorting @return {ComputedProperty} computes a new sorted array based