From 17d88d3626daa260b51872858d0caedaa8de5ee4 Mon Sep 17 00:00:00 2001 From: Marten Schilstra Date: Tue, 14 Jul 2015 21:08:03 +0200 Subject: [PATCH 1/2] [CLEANUP beta] Cleanup Ember.get - Removes support for global lookup with get. - Removes support for get with this in paths. - Removes support for get with null as first parameter. - Adds an assertion that get always has to be called with two parameters. --- packages/ember-metal/lib/property_get.js | 22 ++---- .../tests/accessors/get_path_test.js | 67 ------------------- .../ember-metal/tests/accessors/get_test.js | 43 +++++++++--- packages/ember-metal/tests/computed_test.js | 67 +------------------ .../mixins/observable/observable_test.js | 31 +-------- 5 files changed, 44 insertions(+), 186 deletions(-) diff --git a/packages/ember-metal/lib/property_get.js b/packages/ember-metal/lib/property_get.js index 1e6031bf568..5975ef5bce6 100644 --- a/packages/ember-metal/lib/property_get.js +++ b/packages/ember-metal/lib/property_get.js @@ -11,7 +11,6 @@ import { hasThis as pathHasThis } from 'ember-metal/path_cache'; import { symbol } from 'ember-metal/utils'; -import isNone from 'ember-metal/is_none'; var FIRST_KEY = /^([^\.]+)/; @@ -51,24 +50,17 @@ export let UNHANDLED_GET = symbol('UNHANDLED_GET'); @public */ export function get(obj, keyName) { + Ember.assert(`Get must be called with two arguments; an object and a property key`, arguments.length === 2); + Ember.assert(`Cannot call get with '${keyName}' on an undefined object.`, obj !== undefined && obj !== null); + Ember.assert(`The key provided to get must be a string, you passed ${keyName}`, typeof keyName === 'string'); + Ember.assert(`'this' in paths is not supported`, !pathHasThis(keyName)); + // Helpers that operate with 'this' within an #each if (keyName === '') { return obj; } - if (!keyName && 'string' === typeof obj) { - keyName = obj; - obj = Ember.lookup; - } - - Ember.assert(`Cannot call get with ${keyName} key.`, !!keyName); - Ember.assert(`Cannot call get with '${keyName}' on an undefined object.`, obj !== undefined); - - if (isNone(obj)) { - return _getPath(obj, keyName); - } - - if (obj && typeof obj[INTERCEPT_GET] === 'function') { + if (typeof obj[INTERCEPT_GET] === 'function') { let result = obj[INTERCEPT_GET](obj, keyName); if (result !== UNHANDLED_GET) { return result; } } @@ -169,7 +161,7 @@ export function _getPath(root, path) { parts = path.split('.'); len = parts.length; for (idx = 0; root != null && idx < len; idx++) { - root = get(root, parts[idx], true); + root = get(root, parts[idx]); if (root && root.isDestroyed) { return undefined; } } return root; diff --git a/packages/ember-metal/tests/accessors/get_path_test.js b/packages/ember-metal/tests/accessors/get_path_test.js index 9789c7bf367..9c6092aebca 100644 --- a/packages/ember-metal/tests/accessors/get_path_test.js +++ b/packages/ember-metal/tests/accessors/get_path_test.js @@ -1,5 +1,3 @@ -/*globals Foo:true $foo:true */ - import { get } from 'ember-metal/property_get'; var obj; @@ -22,27 +20,10 @@ var moduleOpts = { nar: 'foo' } }; - - window.Foo = { - bar: { - baz: { biff: 'FooBiff' } - } - }; - - window.aProp = 'aPropy'; - - window.$foo = { - bar: { - baz: { biff: '$FOOBIFF' } - } - }; }, teardown() { obj = undefined; - window.Foo = undefined; - window.aProp = undefined; - window.$foo = undefined; } }; @@ -64,18 +45,6 @@ QUnit.test('[obj, foothis.bar] -> obj.foothis.bar', function() { deepEqual(get(obj, 'foothis.bar'), obj.foothis.bar); }); -QUnit.test('[obj, this.foo] -> obj.foo', function() { - deepEqual(get(obj, 'this.foo'), obj.foo); -}); - -QUnit.test('[obj, this.foo.bar] -> obj.foo.bar', function() { - deepEqual(get(obj, 'this.foo.bar'), obj.foo.bar); -}); - -QUnit.test('[obj, this.Foo.bar] -> (undefined)', function() { - equal(get(obj, 'this.Foo.bar'), undefined); -}); - QUnit.test('[obj, falseValue.notDefined] -> (undefined)', function() { equal(get(obj, 'falseValue.notDefined'), undefined); }); @@ -103,39 +72,3 @@ QUnit.test('[obj, Foo] -> (undefined)', function() { QUnit.test('[obj, Foo.bar] -> (undefined)', function() { equal(get(obj, 'Foo.bar'), undefined); }); - -// .......................................................... -// NULL TARGET -// - -QUnit.test('[null, Foo] -> Foo', function() { - equal(get(null, 'Foo'), Foo); -}); - -QUnit.test('[null, Foo.bar] -> Foo.bar', function() { - deepEqual(get(null, 'Foo.bar'), Foo.bar); -}); - -QUnit.test('[null, $foo] -> $foo', function() { - equal(get(null, '$foo'), window.$foo); -}); - -QUnit.test('[null, aProp] -> null', function() { - equal(get(null, 'aProp'), null); -}); - -// .......................................................... -// NO TARGET -// - -QUnit.test('[Foo] -> Foo', function() { - deepEqual(get('Foo'), Foo); -}); - -QUnit.test('[aProp] -> aProp', function() { - deepEqual(get('aProp'), window.aProp); -}); - -QUnit.test('[Foo.bar] -> Foo.bar', function() { - deepEqual(get('Foo.bar'), Foo.bar); -}); diff --git a/packages/ember-metal/tests/accessors/get_test.js b/packages/ember-metal/tests/accessors/get_test.js index 96269edbbda..b5794d72924 100644 --- a/packages/ember-metal/tests/accessors/get_test.js +++ b/packages/ember-metal/tests/accessors/get_test.js @@ -95,6 +95,24 @@ testBoth('should call unknownProperty on watched values if the value is undefine equal(get(obj, 'foo'), 'FOO', 'should return value from unknown'); }); +QUnit.test('warn on attemps to call get with no arguments', function() { + expectAssertion(function() { + get('aProperty'); + }, /Get must be called with two arguments;/i); +}); + +QUnit.test('warn on attemps to call get with only one argument', function() { + expectAssertion(function() { + get('aProperty'); + }, /Get must be called with two arguments;/i); +}); + +QUnit.test('warn on attemps to call get with more then two arguments', function() { + expectAssertion(function() { + get({}, 'aProperty', true); + }, /Get must be called with two arguments;/i); +}); + QUnit.test('warn on attempts to get a property of undefined', function() { expectAssertion(function() { get(undefined, 'aProperty'); @@ -107,28 +125,35 @@ QUnit.test('warn on attempts to get a property path of undefined', function() { }, /Cannot call get with 'aProperty.on.aPath' on an undefined object/); }); -QUnit.test('returns null when fetching a complex local path on a null context', function() { - equal(get(null, 'aProperty.on.aPath'), null); +QUnit.test('warn on attempts to get a property of null', function() { + expectAssertion(function() { + get(null, 'aProperty'); + }, /Cannot call get with 'aProperty' on an undefined object/); }); -QUnit.test('returns null when fetching a simple local path on a null context', function() { - equal(get(null, 'aProperty'), null); +QUnit.test('warn on attempts to get a property path of null', function() { + expectAssertion(function() { + get(null, 'aProperty.on.aPath'); + }, /Cannot call get with 'aProperty.on.aPath' on an undefined object/); }); -QUnit.test('warn on attempts to get a falsy property', function() { +QUnit.test('warn on attempts to use get with an unsupported property path', function() { var obj = {}; expectAssertion(function() { get(obj, null); - }, /Cannot call get with null key/); + }, /The key provided to get must be a string, you passed null/); expectAssertion(function() { get(obj, NaN); - }, /Cannot call get with NaN key/); + }, /The key provided to get must be a string, you passed NaN/); expectAssertion(function() { get(obj, undefined); - }, /Cannot call get with undefined key/); + }, /The key provided to get must be a string, you passed undefined/); expectAssertion(function() { get(obj, false); - }, /Cannot call get with false key/); + }, /The key provided to get must be a string, you passed false/); + expectAssertion(function() { + get(obj, 42); + }, /The key provided to get must be a string, you passed 42/); }); // .......................................................... diff --git a/packages/ember-metal/tests/computed_test.js b/packages/ember-metal/tests/computed_test.js index c4855fbac86..777bd217b88 100644 --- a/packages/ember-metal/tests/computed_test.js +++ b/packages/ember-metal/tests/computed_test.js @@ -18,8 +18,7 @@ import { _addBeforeObserver } from 'ember-metal/observer'; -var originalLookup = Ember.lookup; -var obj, count, Global, lookup; +var obj, count; QUnit.module('computed'); @@ -444,9 +443,6 @@ testBoth('throws assertion if brace expansion notation has spaces', function (ge var func; var moduleOpts = { setup() { - originalLookup = Ember.lookup; - lookup = Ember.lookup = {}; - obj = { foo: { bar: { @@ -457,18 +453,6 @@ var moduleOpts = { } }; - Global = { - foo: { - bar: { - baz: { - biff: 'BIFF' - } - } - } - }; - - lookup['Global'] = Global; - count = 0; func = function() { count++; @@ -477,8 +461,7 @@ var moduleOpts = { }, teardown() { - obj = count = func = Global = null; - Ember.lookup = originalLookup; + obj = count = func = null; } }; @@ -528,52 +511,6 @@ testBoth('depending on simple chain', function(get, set) { equal(count, 8, 'should be not have invoked computed again'); }); -testBoth('depending on Global chain', function(get, set) { - // assign computed property - defineProperty(obj, 'prop', computed(function() { - count++; - return get('Global.foo.bar.baz.biff') + ' ' + count; - }).property('Global.foo.bar.baz.biff')); - - equal(get(obj, 'prop'), 'BIFF 1'); - - set(get(Global, 'foo.bar.baz'), 'biff', 'BUZZ'); - equal(get(obj, 'prop'), 'BUZZ 2'); - equal(get(obj, 'prop'), 'BUZZ 2'); - - set(get(Global, 'foo.bar'), 'baz', { biff: 'BLOB' }); - equal(get(obj, 'prop'), 'BLOB 3'); - equal(get(obj, 'prop'), 'BLOB 3'); - - set(get(Global, 'foo.bar.baz'), 'biff', 'BUZZ'); - equal(get(obj, 'prop'), 'BUZZ 4'); - equal(get(obj, 'prop'), 'BUZZ 4'); - - set(get(Global, 'foo'), 'bar', { baz: { biff: 'BOOM' } }); - equal(get(obj, 'prop'), 'BOOM 5'); - equal(get(obj, 'prop'), 'BOOM 5'); - - set(get(Global, 'foo.bar.baz'), 'biff', 'BUZZ'); - equal(get(obj, 'prop'), 'BUZZ 6'); - equal(get(obj, 'prop'), 'BUZZ 6'); - - set(Global, 'foo', { bar: { baz: { biff: 'BLARG' } } }); - equal(get(obj, 'prop'), 'BLARG 7'); - equal(get(obj, 'prop'), 'BLARG 7'); - - set(get(Global, 'foo.bar.baz'), 'biff', 'BUZZ'); - equal(get(obj, 'prop'), 'BUZZ 8'); - equal(get(obj, 'prop'), 'BUZZ 8'); - - defineProperty(obj, 'prop'); - set(obj, 'prop', 'NONE'); - equal(get(obj, 'prop'), 'NONE'); - - set(Global, 'foo', { bar: { baz: { biff: 'BLARG' } } }); - equal(get(obj, 'prop'), 'NONE'); // should do nothing - equal(count, 8, 'should be not have invoked computed again'); -}); - testBoth('chained dependent keys should evaluate computed properties lazily', function(get, set) { defineProperty(obj.foo.bar, 'b', computed(func)); defineProperty(obj.foo, 'c', computed(function() {}).property('bar.b')); diff --git a/packages/ember-runtime/tests/legacy_1x/mixins/observable/observable_test.js b/packages/ember-runtime/tests/legacy_1x/mixins/observable/observable_test.js index 25fe5992505..9dbf61f3d7f 100644 --- a/packages/ember-runtime/tests/legacy_1x/mixins/observable/observable_test.js +++ b/packages/ember-runtime/tests/legacy_1x/mixins/observable/observable_test.js @@ -155,29 +155,10 @@ QUnit.test('raise if the provided object is undefined', function() { }); QUnit.test('should work when object is Ember (used in Ember.get)', function() { - equal(get('Ember.RunLoop'), Ember.RunLoop, 'Ember.get'); equal(get(Ember, 'RunLoop'), Ember.RunLoop, 'Ember.get(Ember, RunLoop)'); }); -QUnit.module('Ember.get() with paths', { - setup() { - lookup = Ember.lookup = {}; - }, - - teardown() { - Ember.lookup = originalLookup; - } -}); - -QUnit.test('should return a property at a given path relative to the lookup', function() { - lookup.Foo = ObservableObject.extend({ - Bar: ObservableObject.extend({ - Baz: computed(function() { return 'blargh'; }).volatile() - }).create() - }).create(); - - equal(get('Foo.Bar.Baz'), 'blargh'); -}); +QUnit.module('Ember.get() with paths'); QUnit.test('should return a property at a given path relative to the passed object', function() { var foo = ObservableObject.create({ @@ -189,16 +170,6 @@ QUnit.test('should return a property at a given path relative to the passed obje equal(get(foo, 'bar.baz'), 'blargh'); }); -QUnit.test('should return a property at a given path relative to the lookup - JavaScript hash', function() { - lookup.Foo = { - Bar: { - Baz: 'blargh' - } - }; - - equal(get('Foo.Bar.Baz'), 'blargh'); -}); - QUnit.test('should return a property at a given path relative to the passed object - JavaScript hash', function() { var foo = { bar: { From e3208b1e04c7e3ba33ba882408e93ec519cba5a8 Mon Sep 17 00:00:00 2001 From: Marten Schilstra Date: Wed, 15 Jul 2015 09:49:48 +0200 Subject: [PATCH 2/2] [CLEANUP beta] Add test that predicate is not null in streams#shouldDisplay --- packages/ember-views/lib/streams/should_display.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ember-views/lib/streams/should_display.js b/packages/ember-views/lib/streams/should_display.js index 5e63831b8b9..a1076b1a0d2 100644 --- a/packages/ember-views/lib/streams/should_display.js +++ b/packages/ember-views/lib/streams/should_display.js @@ -14,7 +14,7 @@ export default function shouldDisplay(predicate) { if (type === 'boolean') { return predicate; } - if (type && type === 'object') { + if (type && type === 'object' && predicate !== null) { var isTruthy = get(predicate, 'isTruthy'); if (typeof isTruthy === 'boolean') { return isTruthy;