[BUGFIX release] Fix descriptorFor to support fallback

Ensure all fallback cases are deprecated. Fixes #16427 and add test for `notifyPropertyChange` not working for plain obj.
emberjs · May 18, 2018 · 8a3947d · 8a3947d
1 parent 02fc941
commit 8a3947d
Show file tree

Hide file tree

Showing 18 changed files with 90 additions and 79 deletions.
diff --git a/packages/ember-meta/index.ts b/packages/ember-meta/index.ts
@@ -7,6 +7,7 @@ export {
   meta,
   MetaCounters,
   peekMeta,
+  PROXY_CONTENT,
   setMeta,
   UNDEFINED,
 } from './lib/meta';
diff --git a/packages/ember-meta/lib/meta.ts b/packages/ember-meta/lib/meta.ts
@@ -1,8 +1,10 @@
-import { assert } from '@ember/debug';
+import { assert, deprecate } from '@ember/debug';
 import { DEBUG } from '@glimmer/env';
 import { Tag } from '@glimmer/reference';
 import { ENV } from 'ember-environment';
-import { lookupDescriptor, symbol, toString } from 'ember-utils';
+import { HAS_NATIVE_PROXY, lookupDescriptor, symbol, toString } from 'ember-utils';
+
+export const PROXY_CONTENT = symbol('PROXY_CONTENT');
 
 export interface MetaCounters {
   peekCalls: number;
@@ -787,10 +789,33 @@ export function descriptorFor(obj: object, keyName: string, _meta?: Meta) {
   );
 
   let meta = _meta === undefined ? peekMeta(obj) : _meta;
-
+  let descriptor;
   if (meta !== undefined) {
-    return meta.peekDescriptors(keyName);
+    descriptor = meta.peekDescriptors(keyName);
+  }
+  if (descriptor === undefined) {
+    let possibleDesc;
+    if (DEBUG && HAS_NATIVE_PROXY && obj[PROXY_CONTENT] !== undefined) {
+      possibleDesc = obj[PROXY_CONTENT][keyName];
+    } else {
+      possibleDesc = obj[keyName];
+    }
+    if (isDescriptor(possibleDesc)) {
+      deprecate(
+        `[DEPRECATED] computed property '${keyName}' was not set on object '${obj &&
+          toString(obj)}' via 'defineProperty'`,
+        false,
+        {
+          id: 'ember-meta.descriptor-on-object',
+          until: '3.5.0',
+          url:
+            'https://emberjs.com/deprecations/v3.x#toc_use-defineProperty-to-define-computed-properties',
+        }
+      );
+      descriptor = possibleDesc;
+    }
   }
+  return descriptor;
 }
 
 /**
@@ -802,8 +827,12 @@ export function descriptorFor(obj: object, keyName: string, _meta?: Meta) {
   @private
 */
 export function isDescriptor(possibleDesc: any | undefined | null): boolean {
-  // TODO make this return possibleDesc is Descriptor
-  return possibleDesc !== null && typeof possibleDesc === 'object' && possibleDesc.isDescriptor;
+  return (
+    possibleDesc !== undefined &&
+    possibleDesc !== null &&
+    typeof possibleDesc === 'object' &&
+    possibleDesc.isDescriptor === true
+  );
 }
 
 export { counters };

diff --git a/packages/ember-metal/index.js b/packages/ember-metal/index.js
@@ -2,8 +2,7 @@ export { default as computed, ComputedProperty, _globalsComputed } from './lib/c
 export { getCacheFor, getCachedValueFor, peekCacheFor } from './lib/computed_cache';
 export { default as alias } from './lib/alias';
 export { deprecateProperty } from './lib/deprecate_property';
-export { descriptorFor, meta, peekMeta, deleteMeta } from 'ember-meta';
-export { PROXY_CONTENT, _getPath, get, getWithDefault } from './lib/property_get';
+export { _getPath, get, getWithDefault } from './lib/property_get';
 export { set, trySet } from './lib/property_set';
 export {
   objectAt,

diff --git a/packages/ember-metal/lib/property_events.js b/packages/ember-metal/lib/property_events.js
@@ -86,8 +86,7 @@ function notifyPropertyChange(obj, keyName, _meta) {
 
   let possibleDesc = descriptorFor(obj, keyName, meta);
 
-  // shouldn't this mean that we're watching this key?
-  if (possibleDesc !== undefined && possibleDesc.didChange) {
+  if (possibleDesc !== undefined && typeof possibleDesc.didChange === 'function') {
     possibleDesc.didChange(obj, keyName);
   }
 

diff --git a/packages/ember-metal/lib/property_get.js b/packages/ember-metal/lib/property_get.js
@@ -2,11 +2,11 @@
 @module @ember/object
 */
 import { DEBUG } from '@glimmer/env';
-import { assert, deprecate } from '@ember/debug';
-import { HAS_NATIVE_PROXY, symbol } from 'ember-utils';
+import { assert } from '@ember/debug';
+import { HAS_NATIVE_PROXY } from 'ember-utils';
 import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
 import { isPath } from './path_cache';
-import { isDescriptor, descriptorFor } from 'ember-meta';
+import { descriptorFor, PROXY_CONTENT } from 'ember-meta';
 import { getCurrentTracker } from './tracked';
 import { tagForProperty } from './tags';
 
@@ -16,8 +16,6 @@ const ALLOWABLE_TYPES = {
   string: true,
 };
 
-export const PROXY_CONTENT = symbol('PROXY_CONTENT');
-
 export let getPossibleMandatoryProxyValue;
 
 if (DEBUG && HAS_NATIVE_PROXY) {
@@ -105,34 +103,15 @@ export function get(obj, keyName) {
     }
 
     descriptor = descriptorFor(obj, keyName);
-
-    if (descriptor === undefined) {
-      if (DEBUG && HAS_NATIVE_PROXY) {
-        value = getPossibleMandatoryProxyValue(obj, keyName);
-      } else {
-        value = obj[keyName];
-      }
-
-      if (isDescriptor(value)) {
-        deprecate(
-          `[DEPRECATED] computed property '${keyName}' was not set on object '${obj &&
-            obj.toString &&
-            obj.toString()}' via 'defineProperty'`,
-          false,
-          {
-            id: 'ember-meta.descriptor-on-object',
-            until: '3.5.0',
-            url:
-              'https://emberjs.com/deprecations/v3.x#toc_use-defineProperty-to-define-computed-properties',
-          }
-        );
-        descriptor = value;
-      }
-    }
-
     if (descriptor !== undefined) {
       return descriptor.get(obj, keyName);
     }
+
+    if (DEBUG && HAS_NATIVE_PROXY) {
+      value = getPossibleMandatoryProxyValue(obj, keyName);
+    } else {
+      value = obj[keyName];
+    }
   } else {
     value = obj[keyName];
   }

diff --git a/packages/ember-metal/lib/property_set.js b/packages/ember-metal/lib/property_set.js
@@ -6,7 +6,7 @@ import { getPossibleMandatoryProxyValue, _getPath as getPath } from './property_
 import { notifyPropertyChange } from './property_events';
 
 import { isPath } from './path_cache';
-import { isDescriptor, peekMeta, descriptorFor } from 'ember-meta';
+import { peekMeta, descriptorFor } from 'ember-meta';
 
 /**
  @module @ember/object
@@ -78,10 +78,7 @@ export function set(obj, keyName, value, tolerant) {
     currentValue = obj[keyName];
   }
 
-  if (isDescriptor(currentValue)) {
-    /* computed property */
-    currentValue.set(obj, keyName, value);
-  } else if (
+  if (
     currentValue === undefined &&
     'object' === typeof obj &&
     !(keyName in obj) &&

diff --git a/packages/ember-metal/tests/accessors/mandatory_setters_test.js b/packages/ember-metal/tests/accessors/mandatory_setters_test.js
@@ -1,5 +1,6 @@
 import { DEBUG } from '@glimmer/env';
-import { get, set, watch, unwatch, meta as metaFor } from '../..';
+import { get, set, watch, unwatch } from '../..';
+import { meta as metaFor } from 'ember-meta';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 function hasMandatorySetter(object, property) {

diff --git a/packages/ember-metal/tests/alias_test.js b/packages/ember-metal/tests/alias_test.js
@@ -3,12 +3,12 @@ import {
   defineProperty,
   get,
   set,
-  meta,
   isWatching,
   addObserver,
   removeObserver,
   tagFor,
 } from '..';
+import { meta } from 'ember-meta';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 let obj, count;

diff --git a/packages/ember-metal/tests/chains_test.js b/packages/ember-metal/tests/chains_test.js
@@ -6,12 +6,11 @@ import {
   defineProperty,
   computed,
   notifyPropertyChange,
-  peekMeta,
-  meta,
   watch,
   unwatch,
   watcherCount,
 } from '..';
+import { meta, peekMeta } from 'ember-meta';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 moduleFor(

diff --git a/packages/ember-metal/tests/computed_test.js b/packages/ember-metal/tests/computed_test.js
@@ -9,8 +9,9 @@ import {
   set,
   isWatching,
   addObserver,
-  meta as metaFor,
+  notifyPropertyChange,
 } from '..';
+import { meta as metaFor } from 'ember-meta';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 let obj, count;
@@ -60,6 +61,28 @@ moduleFor(
       assert.equal(count, 1, 'should have invoked computed property');
     }
 
+    ['@test `notifyPropertyChange` works for plain objects #GH16427'](assert) {
+      let obj = {
+        a: 50,
+        b: computed(function() {
+          return this.a / 5;
+        }),
+      };
+
+      expectDeprecation(function() {
+        assert.equal(get(obj, 'b'), 10);
+      });
+
+      obj.a = 10;
+      expectDeprecation(function() {
+        notifyPropertyChange(obj, 'b');
+      });
+
+      expectDeprecation(function() {
+        assert.equal(get(obj, 'b'), 2);
+      });
+    }
+
     ['@test defining computed property should invoke property on get'](assert) {
       let obj = {};
       let count = 0;

diff --git a/packages/ember-metal/tests/meta_test.js b/packages/ember-metal/tests/meta_test.js
@@ -1,4 +1,4 @@
-import { meta } from '..';
+import { meta } from 'ember-meta';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 moduleFor(

diff --git a/packages/ember-metal/tests/watching/watch_test.js b/packages/ember-metal/tests/watching/watch_test.js
@@ -1,15 +1,6 @@
 import { context } from 'ember-environment';
-import {
-  meta,
-  set,
-  get,
-  computed,
-  defineProperty,
-  addListener,
-  watch,
-  unwatch,
-  deleteMeta,
-} from '../..';
+import { set, get, computed, defineProperty, addListener, watch, unwatch, deleteMeta } from '../..';
+import { meta } from 'ember-meta';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 
 let didCount, didKeys, originalLookup;

diff --git a/packages/ember-runtime/lib/mixins/-proxy.js b/packages/ember-runtime/lib/mixins/-proxy.js
@@ -3,10 +3,10 @@
 */
 
 import { combine, CONSTANT_TAG, DirtyableTag, UpdatableTag } from '@glimmer/reference';
+import { meta } from 'ember-meta';
 import {
   get,
   set,
-  meta,
   addObserver,
   removeObserver,
   notifyPropertyChange,

diff --git a/packages/ember-runtime/lib/system/core_object.js b/packages/ember-runtime/lib/system/core_object.js
@@ -13,18 +13,14 @@ import {
   isInternalSymbol,
 } from 'ember-utils';
 import { schedule } from '@ember/runloop';
+import { descriptorFor, meta, peekMeta, deleteMeta, PROXY_CONTENT } from 'ember-meta';
 import {
-  PROXY_CONTENT,
-  descriptorFor,
-  meta,
-  peekMeta,
   finishChains,
   sendEvent,
   Mixin,
   defineProperty,
   ComputedProperty,
   InjectedProperty,
-  deleteMeta,
   descriptor,
   classToString,
 } from 'ember-metal';

diff --git a/packages/ember-runtime/lib/utils.js b/packages/ember-runtime/lib/utils.js
@@ -1,5 +1,6 @@
 import { DEBUG } from '@glimmer/env';
-import { PROXY_CONTENT, get } from 'ember-metal';
+import { get } from 'ember-metal';
+import { PROXY_CONTENT } from 'ember-meta';
 import { HAS_NATIVE_PROXY } from 'ember-utils';
 import EmberArray, { A } from './mixins/array';
 import EmberObject from './system/object';

diff --git a/packages/ember-runtime/tests/system/object/destroy_test.js b/packages/ember-runtime/tests/system/object/destroy_test.js
@@ -1,12 +1,6 @@
 import { run } from '@ember/runloop';
-import {
-  observer,
-  get,
-  set,
-  beginPropertyChanges,
-  endPropertyChanges,
-  peekMeta,
-} from 'ember-metal';
+import { observer, get, set, beginPropertyChanges, endPropertyChanges } from 'ember-metal';
+import { peekMeta } from 'ember-meta';
 import EmberObject from '../../../lib/system/object';
 import { DEBUG } from '@glimmer/env';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';

diff --git a/packages/ember-views/lib/mixins/class_names_support.js b/packages/ember-views/lib/mixins/class_names_support.js
@@ -2,7 +2,8 @@
 @module ember
 */
 
-import { Mixin, descriptorFor } from 'ember-metal';
+import { descriptorFor } from 'ember-meta';
+import { Mixin } from 'ember-metal';
 import { assert } from '@ember/debug';
 
 const EMPTY_ARRAY = Object.freeze([]);

diff --git a/packages/ember-views/lib/mixins/view_support.js b/packages/ember-views/lib/mixins/view_support.js
@@ -1,5 +1,6 @@
 import { guidFor } from 'ember-utils';
-import { descriptor, descriptorFor, Mixin } from 'ember-metal';
+import { descriptorFor } from 'ember-meta';
+import { descriptor, Mixin } from 'ember-metal';
 import { assert } from '@ember/debug';
 import { hasDOM } from 'ember-browser-environment';
 import { matches } from '../system/utils';