fix(utils/decorator): handle desc return value

packages/-ember-decorators/package.json

@@ -68,6 +68,7 @@
     "ember-load-initializers": "^1.0.0",
     "ember-maybe-import-regenerator": "^0.1.6",
     "ember-native-dom-helpers": "^0.5.0",
+    "ember-qunit-assert-helpers": "^0.2.1",
     "ember-resolver": "^4.0.0",
     "ember-root-url": "^0.2.0",
     "ember-source": "~3.0.0",

packages/-ember-decorators/tests/unit/utils/decorator-helpers-test.js

@@ -1,12 +1,55 @@
 import { DEBUG } from '@glimmer/env';
 
 import { module, test } from 'qunit';
-import { decoratorWithParams, decoratorWithRequiredParams } from '@ember-decorators/utils/decorator';
+import {
+  decorator,
+  decoratorWithParams,
+  decoratorWithRequiredParams,
+} from '@ember-decorators/utils/decorator';
 
 module('decorator helpers', function() {
+  module('decorator', function() {
+    test('it works with returning a newly created member descriptor', function(assert) {
+      let decorate = decorator(desc => {
+        return {
+          ...desc,
+          descriptor: {
+            configurable: desc.configurable,
+            enumerable: desc.enumerable,
+            get() {
+              return 1337;
+            },
+          },
+        };
+      });
 
-  module('decoratorWithParams', function() {
+      class Foo {
+        @decorate foo() {}
+      }
+
+      let foo = new Foo();
+      assert.strictEqual(foo.foo, 1337);
+    });
+
+    test('it warns about deprecated descriptor mutation by reference', function(assert) {
+      let decorate = decorator(desc => {
+        delete desc.descriptor.value;
+        delete desc.descriptor.writable;
+        desc.descriptor.get = () => 1337;
+      });
+
+      assert.expectDeprecation(() => {
+        class Foo {
+          @decorate foo() {}
+        }
+
+        let foo = new Foo();
+        assert.strictEqual(foo.foo, 1337);
+      }, /Directly mutating the descriptor by reference is deprecated. Return it instead./);
+    });
+  });
 
+  module('decoratorWithParams', function() {
     test('it works with params', function(assert) {
       let decorate = decoratorWithParams(({ key }, params) => {
         assert.equal(key, 'foo', 'correct key decorated');
@@ -51,7 +94,6 @@ module('decorator helpers', function() {
   });
 
   module('decoratorWithRequiredParams', function() {
-
     test('it works with params', function(assert) {
       let decorate = decoratorWithRequiredParams(({ key }, params) => {
         assert.equal(key, 'foo', 'correct key decorated');
@@ -69,37 +111,31 @@ module('decorator helpers', function() {
 
     if (DEBUG) {
       test('it throws without params', function(assert) {
-        assert.throws(
-          () => {
-            let decorate = decoratorWithRequiredParams(() => {
-              assert.ok(false, 'decorator ran');
-            }, 'decorate');
+        assert.throws(() => {
+          let decorate = decoratorWithRequiredParams(() => {
+            assert.ok(false, 'decorator ran');
+          }, 'decorate');
 
-            class Foo {
-              @decorate foo() {}
-            }
+          class Foo {
+            @decorate foo() {}
+          }
 
-            new Foo();
-          },
-          /The @decorate decorator requires parameters/
-        );
+          new Foo();
+        }, /The @decorate decorator requires parameters/);
       });
 
       test('it throws with empty params list', function(assert) {
-        assert.throws(
-          () => {
-            let decorate = decoratorWithRequiredParams(() => {
-              assert.ok(false, 'decorator ran');
-            }, 'decorate');
+        assert.throws(() => {
+          let decorate = decoratorWithRequiredParams(() => {
+            assert.ok(false, 'decorator ran');
+          }, 'decorate');
 
-            class Foo {
-              @decorate() foo() {}
-            }
+          class Foo {
+            @decorate() foo() {}
+          }
 
-            new Foo();
-          },
-          /The @decorate decorator requires parameters/
-        );
+          new Foo();
+        }, /The @decorate decorator requires parameters/);
       });
     }
   });

packages/component/addon/index.js

@@ -1,7 +1,10 @@
 import { assert } from '@ember/debug';
 
 import collapseProto from '@ember-decorators/utils/collapse-proto';
-import { decoratorWithParams, decoratorWithRequiredParams } from '@ember-decorators/utils/decorator';
+import {
+  decoratorWithParams,
+  decoratorWithRequiredParams,
+} from '@ember-decorators/utils/decorator';
 
 /**
   Decorator which indicates that the field or computed should be bound
@@ -38,28 +41,31 @@ export const attribute = decoratorWithParams((desc, params = []) => {
     params.every(s => typeof s === 'string')
   );
 
-  desc.finisher = target => {
-    let { prototype } = target;
-    let { key, descriptor } = desc;
+  return {
+    ...desc,
+    finisher(target) {
+      let { prototype } = target;
+      let { key, descriptor } = desc;
 
-    collapseProto(prototype);
+      collapseProto(prototype);
 
-    if (!prototype.hasOwnProperty('attributeBindings')) {
-      let parentValue = prototype.attributeBindings;
-      prototype.attributeBindings = Array.isArray(parentValue) ? parentValue.slice() : [];
-    }
+      if (!prototype.hasOwnProperty('attributeBindings')) {
+        let parentValue = prototype.attributeBindings;
+        prototype.attributeBindings = Array.isArray(parentValue) ? parentValue.slice() : [];
+      }
 
-    let binding = params[0] ? `${key}:${params[0]}` : key;
+      let binding = params[0] ? `${key}:${params[0]}` : key;
 
-    prototype.attributeBindings.push(binding);
+      prototype.attributeBindings.push(binding);
 
-    if (descriptor) {
-      // Decorated fields are currently not configurable in Babel for some reason, so ensure
-      // that the field becomes configurable (else it messes with things)
-      descriptor.configurable = true;
-    }
+      if (descriptor) {
+        // Decorated fields are currently not configurable in Babel for some reason, so ensure
+        // that the field becomes configurable (else it messes with things)
+        descriptor.configurable = true;
+      }
 
-    return target;
+      return target;
+    },
   };
 });
 
@@ -101,28 +107,31 @@ export const className = decoratorWithParams((desc, params = []) => {
     params.every(s => typeof s === 'string')
   );
 
-  desc.finisher = target => {
-    let { prototype } = target;
-    let { key, descriptor } = desc;
+  return {
+    ...desc,
+    finisher(target) {
+      let { prototype } = target;
+      let { key, descriptor } = desc;
 
-    collapseProto(prototype);
+      collapseProto(prototype);
 
-    if (!prototype.hasOwnProperty('classNameBindings')) {
-      let parentValue = prototype.classNameBindings;
-      prototype.classNameBindings = Array.isArray(parentValue) ? parentValue.slice() : [];
-    }
+      if (!prototype.hasOwnProperty('classNameBindings')) {
+        let parentValue = prototype.classNameBindings;
+        prototype.classNameBindings = Array.isArray(parentValue) ? parentValue.slice() : [];
+      }
 
-    let binding = params.length > 0 ? `${key}:${params.join(':')}` : key;
+      let binding = params.length > 0 ? `${key}:${params.join(':')}` : key;
 
-    prototype.classNameBindings.push(binding);
+      prototype.classNameBindings.push(binding);
 
-    if (descriptor) {
-      // Decorated fields are currently not configurable in Babel for some reason, so ensure
-      // that the field becomes configurable (else it messes with things)
-      descriptor.configurable = true;
-    }
+      if (descriptor) {
+        // Decorated fields are currently not configurable in Babel for some reason, so ensure
+        // that the field becomes configurable (else it messes with things)
+        descriptor.configurable = true;
+      }
 
-    return target;
+      return target;
+    },
   };
 });
 
@@ -144,19 +153,22 @@ export const classNames = decoratorWithRequiredParams((desc, classNames) => {
     classNames.reduce((allStrings, name) => allStrings && typeof name === 'string', true)
   );
 
-  desc.finisher = target => {
-    let { prototype } = target;
+  return {
+    ...desc,
+    finisher(target) {
+      let { prototype } = target;
 
-    collapseProto(prototype);
+      collapseProto(prototype);
 
-    if ('classNames' in prototype) {
-      let parentClasses = prototype.classNames;
-      classNames.unshift(...parentClasses);
-    }
+      if ('classNames' in prototype) {
+        let parentClasses = prototype.classNames;
+        classNames.unshift(...parentClasses);
+      }
 
-    prototype.classNames = classNames;
+      prototype.classNames = classNames;
 
-    return target;
+      return target;
+    },
   };
 }, 'classNames');
 
@@ -183,9 +195,12 @@ export const tagName = decoratorWithRequiredParams((desc, params) => {
     typeof tagName === 'string'
   );
 
-  desc.finisher = target => {
-    target.prototype.tagName = tagName;
-    return target;
+  return {
+    ...desc,
+    finisher(target) {
+      target.prototype.tagName = tagName;
+      return target;
+    },
   };
 }, 'tagName');
 
@@ -230,8 +245,11 @@ export const layout = decoratorWithRequiredParams((desc, params) => {
     (() => typeof template === 'object' && typeof template.indexOf === 'undefined')()
   );
 
-  desc.finisher = target => {
-    target.prototype.layout = template;
-    return target;
+  return {
+    ...desc,
+    finisher(target) {
+      target.prototype.layout = template;
+      return target;
+    },
   };
 }, 'layout');