From 2a0272453472353cc7dbbe7a630294d06cf89a31 Mon Sep 17 00:00:00 2001 From: Matthew Beale Date: Tue, 3 Dec 2013 20:43:23 -0500 Subject: [PATCH] [BREAKING BUGFIX] Do not assume null Ember.get targets are all globals * Changes globals to only mean properties starting with $ or a capital letter. * Changes normalizeTuple(obj, 'Foo') to resolve Foo as a global. This makes the behavior consistent with normalizeTuple(obj, 'Foo.bar') which already resolves Foo as a global (and looks up bar). * Changes normalizeTuple(null, 'Foo') to resolve Foo as a global. * Specs normalizeTuple('null', 'foo') to resolve to [undefined, '']. This is unfortunate but needed to keep getPath happy. This code is old, and tricky to modify due to perf optimizations and edge case behavior. It would be great to refactor more heavily at some other date. I've spec'd out more of the current behavior and edge cases in this commit. Fixes #3760 --- CHANGELOG.md | 4 + packages/ember-metal/lib/path_cache.js | 4 +- packages/ember-metal/lib/property_get.js | 45 +++++------ packages/ember-metal/lib/property_set.js | 25 +++--- .../tests/accessors/get_path_test.js | 77 ++++++++++++------- .../ember-metal/tests/accessors/get_test.js | 8 ++ .../tests/accessors/normalize_tuple_test.js | 28 +++++-- .../tests/accessors/set_path_test.js | 7 +- 8 files changed, 117 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d37b7252fff..6e7842285e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Ember Changelog +### Canary + +- [#3852](https://github.com/emberjs/ember.js/pull/3852) [BREAKING BUGFIX] Do not assume null Ember.get targets always refer to a global + ### 1.11.0-beta.1 (February 06, 2015) - [#10160](https://github.com/emberjs/ember.js/pull/10160) [FEATURE] Add index as an optional parameter to #each blocks [@tim-evans](https://github.com/tim-evans) diff --git a/packages/ember-metal/lib/path_cache.js b/packages/ember-metal/lib/path_cache.js index 168d3a4cf2e..243a489cf2a 100644 --- a/packages/ember-metal/lib/path_cache.js +++ b/packages/ember-metal/lib/path_cache.js @@ -1,7 +1,7 @@ import Cache from 'ember-metal/cache'; -var IS_GLOBAL = /^([A-Z$]|([0-9][A-Z$]))/; -var IS_GLOBAL_PATH = /^([A-Z$]|([0-9][A-Z$])).*[\.]/; +var IS_GLOBAL = /^[A-Z$]/; +var IS_GLOBAL_PATH = /^[A-Z$].*[\.]/; var HAS_THIS = 'this.'; var isGlobalCache = new Cache(1000, function(key) { return IS_GLOBAL.test(key); }); diff --git a/packages/ember-metal/lib/property_get.js b/packages/ember-metal/lib/property_get.js index f3a341d8db6..f9273ceaa9e 100644 --- a/packages/ember-metal/lib/property_get.js +++ b/packages/ember-metal/lib/property_get.js @@ -5,7 +5,7 @@ import Ember from "ember-metal/core"; import EmberError from "ember-metal/error"; import { - isGlobalPath, + isGlobal as detectIsGlobal, isPath, hasThis as pathHasThis } from "ember-metal/path_cache"; @@ -52,19 +52,14 @@ export function get(obj, keyName) { if (!keyName && 'string' === typeof obj) { keyName = obj; - obj = null; + 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 (obj === null) { - var value = _getPath(obj, keyName); - Ember.deprecate( - "Ember.get fetched '"+keyName+"' from the global context. This behavior will change in the future (issue #3852)", - !value || (obj && obj !== Ember.lookup) || isPath(keyName) || isGlobalPath(keyName+".") // Add a . to ensure simple paths are matched. - ); - return value; + if (!obj) { + return _getPath(obj, keyName); } var meta = obj['__ember_meta__']; @@ -113,46 +108,42 @@ export function get(obj, keyName) { */ export function normalizeTuple(target, path) { var hasThis = pathHasThis(path); - var isGlobal = !hasThis && isGlobalPath(path); + var isGlobal = !hasThis && detectIsGlobal(path); var key; - if (!target || isGlobal) { - target = Ember.lookup; + if (!target && !isGlobal) { + return [undefined, '']; } if (hasThis) { path = path.slice(5); } - Ember.deprecate( - "normalizeTuple will return '"+path+"' as a non-global. This behavior will change in the future (issue #3852)", - target === Ember.lookup || !target || hasThis || isGlobal || !isGlobalPath(path+'.') - ); + if (!target || isGlobal) { + target = Ember.lookup; + } - if (target === Ember.lookup) { + if (isGlobal && isPath(path)) { key = path.match(FIRST_KEY)[0]; target = get(target, key); path = path.slice(key.length+1); } // must return some kind of path to be valid else other things will break. - if (!path || path.length===0) { - throw new EmberError('Path cannot be empty'); - } + validateIsPath(path); return [target, path]; } +function validateIsPath(path) { + if (!path || path.length===0) { + throw new EmberError('Object in path '+path+' could not be found or was destroyed.'); + } +} + export function _getPath(root, path) { var hasThis, parts, tuple, idx, len; - // If there is no root and path is a key name, return that - // property from the global object. - // E.g. get('Ember') -> Ember - if (root === null && !isPath(path)) { - return get(Ember.lookup, path); - } - // detect complicated paths and normalize them hasThis = pathHasThis(path); diff --git a/packages/ember-metal/lib/property_set.js b/packages/ember-metal/lib/property_set.js index 38c9428b390..f2c8084d865 100644 --- a/packages/ember-metal/lib/property_set.js +++ b/packages/ember-metal/lib/property_set.js @@ -7,12 +7,11 @@ import { import { defineProperty } from "ember-metal/properties"; import EmberError from "ember-metal/error"; import { - isPath + isPath, + isGlobalPath } from "ember-metal/path_cache"; import { hasPropertyAccessors } from "ember-metal/platform/define_property"; -var IS_GLOBAL = /^([A-Z$]|([0-9][A-Z$]))/; - /** Sets the value of a property on an object, respecting computed properties and notifying observers and other listeners of the change. If the @@ -28,25 +27,27 @@ var IS_GLOBAL = /^([A-Z$]|([0-9][A-Z$]))/; */ export function set(obj, keyName, value, tolerant) { if (typeof obj === 'string') { - Ember.assert("Path '" + obj + "' must be global if no obj is given.", IS_GLOBAL.test(obj)); + Ember.assert("Path '" + obj + "' must be global if no obj is given.", isGlobalPath(obj)); value = keyName; keyName = obj; - obj = null; + obj = Ember.lookup; } Ember.assert("Cannot call set with "+ keyName +" key.", !!keyName); - if (!obj) { + if (obj === Ember.lookup) { return setPath(obj, keyName, value, tolerant); } - var meta = obj['__ember_meta__']; - var possibleDesc = obj[keyName]; - var desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined; + var meta, possibleDesc, desc; + if (obj) { + meta = obj['__ember_meta__']; + possibleDesc = obj[keyName]; + desc = (possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor) ? possibleDesc : undefined; + } var isUnknown, currentValue; - - if (desc === undefined && isPath(keyName)) { + if ((!obj || desc === undefined) && isPath(keyName)) { return setPath(obj, keyName, value, tolerant); } @@ -57,7 +58,7 @@ export function set(obj, keyName, value, tolerant) { desc.set(obj, keyName, value); } else { - if (typeof obj === 'object' && obj !== null && value !== undefined && obj[keyName] === value) { + if (obj !== null && value !== undefined && typeof obj === 'object' && obj[keyName] === value) { return value; } diff --git a/packages/ember-metal/tests/accessors/get_path_test.js b/packages/ember-metal/tests/accessors/get_path_test.js index 3ef9debca77..c8141e38144 100644 --- a/packages/ember-metal/tests/accessors/get_path_test.js +++ b/packages/ember-metal/tests/accessors/get_path_test.js @@ -2,13 +2,6 @@ import { get } from 'ember-metal/property_get'; -function expectGlobalContextDeprecation(assertion) { - expectDeprecation( - assertion, - "Ember.get fetched 'localPathGlobal' from the global context. This behavior will change in the future (issue #3852)" - ); -} - var obj; var moduleOpts = { setup: function() { @@ -23,7 +16,10 @@ var moduleOpts = { baz: { biff: 'BIFF' } } }, - falseValue: false + falseValue: false, + Wuz: { + nar: 'foo' + } }; window.Foo = { @@ -32,20 +28,20 @@ var moduleOpts = { } }; + window.aProp = 'aPropy'; + window.$foo = { bar: { baz: { biff: '$FOOBIFF' } } }; - - window.localPathGlobal = 5; }, teardown: function() { obj = undefined; window.Foo = undefined; + window.aProp = undefined; window.$foo = undefined; - window.localPathGlobal = undefined; } }; @@ -75,39 +71,66 @@ 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] -> (null)', function() { - deepEqual(get(obj, 'this.Foo.bar'), undefined); +QUnit.test('[obj, this.Foo.bar] -> (undefined)', function() { + equal(get(obj, 'this.Foo.bar'), undefined); }); -QUnit.test('[obj, falseValue.notDefined] -> (null)', function() { - deepEqual(get(obj, 'falseValue.notDefined'), undefined); +QUnit.test('[obj, falseValue.notDefined] -> (undefined)', function() { + equal(get(obj, 'falseValue.notDefined'), undefined); }); // .......................................................... -// LOCAL PATHS WITH NO TARGET DEPRECATION +// GLOBAL PATHS TREATED LOCAL WITH GET // -QUnit.test('[null, length] returning data is deprecated', function() { - expectGlobalContextDeprecation(function() { - equal(5, get(null, 'localPathGlobal')); - }); +QUnit.test('[obj, Wuz] -> obj.Wuz', function() { + deepEqual(get(obj, 'Wuz'), obj.Wuz); }); -QUnit.test('[length] returning data is deprecated', function() { - expectGlobalContextDeprecation(function() { - equal(5, get('localPathGlobal')); - }); +QUnit.test('[obj, Wuz.nar] -> obj.Wuz.nar', function() { + deepEqual(get(obj, 'Wuz.nar'), obj.Wuz.nar); +}); + +QUnit.test('[obj, Foo] -> (undefined)', function() { + equal(get(obj, 'Foo'), undefined); +}); + +QUnit.test('[obj, Foo.bar] -> (undefined)', function() { + equal(get(obj, 'Foo.bar'), undefined); }); // .......................................................... -// NO TARGET +// NULL TARGET // QUnit.test('[null, Foo] -> Foo', function() { - deepEqual(get('Foo'), Foo); + equal(get(null, 'Foo'), Foo); }); QUnit.test('[null, Foo.bar] -> Foo.bar', function() { - deepEqual(get('Foo.bar'), Foo.bar); + 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 1187d22a400..cd9c3babb8d 100644 --- a/packages/ember-metal/tests/accessors/get_test.js +++ b/packages/ember-metal/tests/accessors/get_test.js @@ -60,6 +60,14 @@ 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('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 falsy property', function() { var obj = {}; expectAssertion(function() { diff --git a/packages/ember-metal/tests/accessors/normalize_tuple_test.js b/packages/ember-metal/tests/accessors/normalize_tuple_test.js index 74aba6dea09..4d751cf76af 100644 --- a/packages/ember-metal/tests/accessors/normalize_tuple_test.js +++ b/packages/ember-metal/tests/accessors/normalize_tuple_test.js @@ -74,10 +74,8 @@ QUnit.test('[obj, this.Foo.bar] -> [obj, Foo.bar]', function() { // GLOBAL PATHS // -QUnit.test('[obj, Foo] -> [obj, Foo]', function() { - expectDeprecation(function() { - deepEqual(normalizeTuple(obj, 'Foo'), [obj, 'Foo']); - }, "normalizeTuple will return 'Foo' as a non-global. This behavior will change in the future (issue #3852)"); +QUnit.test('[obj, Foo] -> [Ember.lookup, Foo]', function() { + deepEqual(normalizeTuple(obj, 'Foo'), [Ember.lookup, 'Foo']); }); QUnit.test('[obj, Foo.bar] -> [Foo, bar]', function() { @@ -92,12 +90,26 @@ QUnit.test('[obj, $foo.bar.baz] -> [$foo, bar.baz]', function() { // NO TARGET // -QUnit.test('[null, Foo] -> EXCEPTION', function() { - throws(function() { - normalizeTuple(null, 'Foo'); - }, Error); +QUnit.test('[null, Foo] -> [Ember.lookup, Foo]', function() { + deepEqual(normalizeTuple(null, 'Foo'), [Ember.lookup, 'Foo']); }); QUnit.test('[null, Foo.bar] -> [Foo, bar]', function() { deepEqual(normalizeTuple(null, 'Foo.bar'), [Foo, 'bar']); }); + +QUnit.test("[null, foo] -> [undefined, '']", function() { + deepEqual(normalizeTuple(null, 'foo'), [undefined, '']); +}); + +QUnit.test("[null, foo.bar] -> [undefined, '']", function() { + deepEqual(normalizeTuple(null, 'foo'), [undefined, '']); +}); + +QUnit.test('[null, $foo] -> [Ember.lookup, $foo]', function() { + deepEqual(normalizeTuple(null, '$foo'), [Ember.lookup, '$foo']); +}); + +QUnit.test('[null, $foo.bar] -> [$foo, bar]', function() { + deepEqual(normalizeTuple(null, '$foo.bar'), [$foo, 'bar']); +}); diff --git a/packages/ember-metal/tests/accessors/set_path_test.js b/packages/ember-metal/tests/accessors/set_path_test.js index 5dad96f8362..fb4f44c9532 100644 --- a/packages/ember-metal/tests/accessors/set_path_test.js +++ b/packages/ember-metal/tests/accessors/set_path_test.js @@ -91,12 +91,9 @@ QUnit.module("set with path - deprecated", { }); QUnit.test('[null, bla] gives a proper exception message', function() { - var exceptionMessage = 'Property set failed: object in path \"bla\" could not be found or was destroyed.'; - try { + expectAssertion(function() { set(null, 'bla', "BAM"); - } catch(ex) { - equal(ex.message, exceptionMessage); - } + }, /You need to provide an object and key to `set`/); }); QUnit.test('[obj, bla.bla] gives a proper exception message', function() {