Merge pull request #11761 from martndemus/cleanup-ember-set

[CLEANUP beta] Cleanup Ember.set
emberjs · Jul 15, 2015 · 0df6ea0 · 0df6ea0
2 parents 8cd526c + a0103c1
commit 0df6ea0
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 58 deletions.
diff --git a/packages/ember-metal/lib/property_set.js b/packages/ember-metal/lib/property_set.js
@@ -10,7 +10,7 @@ import { defineProperty } from 'ember-metal/properties';
 import EmberError from 'ember-metal/error';
 import {
   isPath,
-  isGlobalPath
+  hasThis as pathHasThis
 } from 'ember-metal/path_cache';
 
 import { symbol } from 'ember-metal/utils';
@@ -32,18 +32,12 @@ export let UNHANDLED_SET = symbol('UNHANDLED_SET');
   @public
 */
 export function set(obj, keyName, value, tolerant) {
-  if (typeof obj === 'string') {
-    Ember.assert(`Path '${obj}' must be global if no obj is given.`, isGlobalPath(obj));
-    value = keyName;
-    keyName = obj;
-    obj = Ember.lookup;
-  }
-
-  Ember.assert(`Cannot call set with '${keyName}' key.`, !!keyName);
-
-  if (obj === Ember.lookup) {
-    return setPath(obj, keyName, value, tolerant);
-  }
+  Ember.assert(
+    `Set must be called with tree or four arguments; an object, a property key, a value and tolerant true/false`,
+    arguments.length === 3 || arguments.length === 4);
+  Ember.assert(`Cannot call set with '${keyName}' on an undefined object.`, obj !== undefined && obj !== null);
+  Ember.assert(`The key provided to set must be a string, you passed ${keyName}`, typeof keyName === 'string');
+  Ember.assert(`'this' in paths is not supported`, !pathHasThis(keyName));
 
   // This path exists purely to implement backwards-compatible
   // effects (specifically, setting a property on a view may
@@ -61,17 +55,16 @@ export function set(obj, keyName, value, tolerant) {
   }
 
   var isUnknown, currentValue;
-  if ((!obj || desc === undefined) && isPath(keyName)) {
+  if (desc === undefined && isPath(keyName)) {
     return setPath(obj, keyName, value, tolerant);
   }
 
-  Ember.assert('You need to provide an object and key to `set`.', !!obj && keyName !== undefined);
   Ember.assert('calling set on destroyed object', !obj.isDestroyed);
 
   if (desc) {
     desc.set(obj, keyName, value);
   } else {
-    if (obj !== null && value !== undefined && typeof obj === 'object' && obj[keyName] === value) {
+    if (value !== undefined && typeof obj === 'object' && obj[keyName] === value) {
       return value;
     }
 

diff --git a/packages/ember-metal/tests/accessors/set_path_test.js b/packages/ember-metal/tests/accessors/set_path_test.js
@@ -2,8 +2,6 @@ import Ember from 'ember-metal/core';
 import { set, trySet } from 'ember-metal/property_set';
 import { get } from 'ember-metal/property_get';
 
-var originalLookup = Ember.lookup;
-
 var obj;
 function commonSetup() {
   obj = {
@@ -13,25 +11,10 @@ function commonSetup() {
       }
     }
   };
-
-  Ember.lookup = {
-    Foo: {
-      bar: {
-        baz: { biff: 'FooBiff' }
-      }
-    },
-
-    $foo: {
-      bar: {
-        baz: { biff: '$FOOBIFF' }
-      }
-    }
-  };
 }
 
 function commonTeardown() {
   obj = null;
-  Ember.lookup = originalLookup;
 }
 
 QUnit.module('set with path', {
@@ -60,25 +43,6 @@ QUnit.test('[obj, foo.bar] -> obj.foo.bar', function() {
   equal(get(obj, 'foo.bar'), 'BAM');
 });
 
-QUnit.test('[obj, this.foo] -> obj.foo', function() {
-  set(obj, 'this.foo', 'BAM');
-  equal(get(obj, 'foo'), 'BAM');
-});
-
-QUnit.test('[obj, this.foo.bar] -> obj.foo.bar', function() {
-  set(obj, 'this.foo.bar', 'BAM');
-  equal(get(obj, 'foo.bar'), 'BAM');
-});
-
-// ..........................................................
-// NO TARGET
-//
-
-QUnit.test('[null, Foo.bar] -> Foo.bar', function() {
-  set(null, 'Foo.bar', 'BAM');
-  equal(get(Ember.lookup.Foo, 'bar'), 'BAM');
-});
-
 // ..........................................................
 // DEPRECATED
 //
@@ -88,12 +52,6 @@ QUnit.module('set with path - deprecated', {
   teardown: commonTeardown
 });
 
-QUnit.test('[null, bla] gives a proper exception message', function() {
-  expectAssertion(function() {
-    set(null, 'bla', 'BAM');
-  }, /You need to provide an object and key to `set`/);
-});
-
 QUnit.test('[obj, bla.bla] gives a proper exception message', function() {
   var exceptionMessage = 'Property set failed: object in path \"bla\" could not be found or was destroyed.';
   try {

diff --git a/packages/ember-metal/tests/accessors/set_test.js b/packages/ember-metal/tests/accessors/set_test.js
@@ -156,3 +156,33 @@ QUnit.test('should call setUnknownProperty if defined and value is undefined', f
   equal(obj.count, 1, 'should have invoked');
 });
 
+QUnit.test('warn on attempts to call set with undefined as object', function() {
+  expectAssertion(function() {
+    set(undefined, 'aProperty', 'BAM');
+  }, /Cannot call set with 'aProperty' on an undefined object./);
+});
+
+QUnit.test('warn on attempts to call set with null as object', function() {
+  expectAssertion(function() {
+    set(null, 'aProperty', 'BAM');
+  }, /Cannot call set with 'aProperty' on an undefined object./);
+});
+
+QUnit.test('warn on attempts to use set with an unsupported property path', function() {
+  var obj = {};
+  expectAssertion(function() {
+    set(obj, null, 42);
+  }, /The key provided to set must be a string, you passed null/);
+  expectAssertion(function() {
+    set(obj, NaN, 42);
+  }, /The key provided to set must be a string, you passed NaN/);
+  expectAssertion(function() {
+    set(obj, undefined, 42);
+  }, /The key provided to set must be a string, you passed undefined/);
+  expectAssertion(function() {
+    set(obj, false, 42);
+  }, /The key provided to set must be a string, you passed false/);
+  expectAssertion(function() {
+    set(obj, 42, 42);
+  }, /The key provided to set must be a string, you passed 42/);
+});