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,58 @@
 import { DEBUG } from '@glimmer/env';
+import { NEEDS_STAGE_1_DECORATORS } from 'ember-decorators-flags';
 
-import { module, test } from 'qunit';
-import { decoratorWithParams, decoratorWithRequiredParams } from '@ember-decorators/utils/decorator';
+import { module, test, skip } from 'qunit';
+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);
+    });
+
+    (NEEDS_STAGE_1_DECORATORS
+      ? test
+      : skip)('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 +97,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 +114,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
@@ -61,6 +64,8 @@ export const attribute = decoratorWithParams((desc, params = []) => {
 
     return target;
   };
+
+  return desc;
 });
 
 /**
@@ -124,6 +129,8 @@ export const className = decoratorWithParams((desc, params = []) => {
 
     return target;
   };
+
+  return desc;
 });
 
 /**
@@ -158,6 +165,8 @@ export const classNames = decoratorWithRequiredParams((desc, classNames) => {
 
     return target;
   };
+
+  return desc;
 }, 'classNames');
 
 /**
@@ -187,6 +196,8 @@ export const tagName = decoratorWithRequiredParams((desc, params) => {
     target.prototype.tagName = tagName;
     return target;
   };
+
+  return desc;
 }, 'tagName');
 
 /**
@@ -234,4 +245,6 @@ export const layout = decoratorWithRequiredParams((desc, params) => {
     target.prototype.layout = template;
     return target;
   };
+
+  return desc;
 }, 'layout');

packages/object/addon/index.js

@@ -58,6 +58,8 @@ export const action = decorator(desc => {
 
     return target;
   };
+
+  return desc;
 });
 
 /**
@@ -165,6 +167,8 @@ export const observes = decoratorWithRequiredParams((desc, params) => {
 
     return target;
   };
+
+  return desc;
 }, 'observes');
 
 /**
@@ -198,6 +202,8 @@ export const unobserves = decoratorWithRequiredParams((desc, params) => {
 
     return target;
   };
+
+  return desc;
 }, 'unobserves');
 
 /**
@@ -232,6 +238,8 @@ export const on = decoratorWithRequiredParams((desc, params) => {
 
     return target;
   };
+
+  return desc;
 }, 'on');
 
 /**
@@ -265,6 +273,8 @@ export const off = decoratorWithRequiredParams((desc, params) => {
 
     return target;
   };
+
+  return desc;
 }, 'off');
 
 /**
@@ -316,6 +326,8 @@ export const readOnly = decorator(desc => {
       modifierMeta[key] = 'readOnly';
     }
   };
+
+  return desc;
 });
 
 /**
@@ -367,4 +379,6 @@ export const volatile = decorator(desc => {
       modifierMeta[key] = 'volatile';
     }
   };
+
+  return desc;
 });

packages/utils/addon/computed.js

@@ -63,6 +63,8 @@ function computedDecoratorInner(fn) {
 
       return target;
     }
+
+    return desc;
   }
 }
 

packages/utils/addon/decorator.js

@@ -1,6 +1,7 @@
 import { assert } from '@ember/debug';
 
 import { NEEDS_STAGE_1_DECORATORS } from 'ember-decorators-flags';
+import { deprecate } from '@ember/application/deprecations';
 
 function isDescriptor(possibleDesc) {
   let isDescriptor = isStage2Descriptor(possibleDesc);
@@ -16,18 +17,16 @@ function isStage1Descriptor(possibleDesc) {
   if (possibleDesc.length === 3) {
     let [target, key, desc] = possibleDesc;
 
-    return typeof target === 'object'
-      && target !== null
-      && typeof key === 'string'
-      && (
-        (
-          typeof desc === 'object'
-          && desc !== null
-          && 'enumerable' in desc
-          && 'configurable' in desc
-        )
-        || desc === undefined // TS compatibility
-      );
+    return (
+      typeof target === 'object' &&
+      target !== null &&
+      typeof key === 'string' &&
+      ((typeof desc === 'object' &&
+        desc !== null &&
+        'enumerable' in desc &&
+        'configurable' in desc) ||
+        desc === undefined) // TS compatibility
+    );
   } else if (possibleDesc.length === 1) {
     let [target] = possibleDesc;
 
@@ -80,17 +79,35 @@ function convertStage1ToStage2(desc) {
   }
 }
 
+function deprecateDirectDescriptorMutation(fn, desc) {
+  let returnValue = fn(desc);
+
+  if (!returnValue) {
+    deprecate(
+      `@ember-decorators/utils: Directly mutating the descriptor by reference is deprecated. Return it instead.`,
+      false,
+      {
+        id: 'ember-decorators.utils.decorator.descriptor-mutation-by-reference',
+        until: '4.0.0',
+      }
+    );
+    return desc;
+  }
+
+  return returnValue;
+}
+
 export function decorator(fn) {
   if (NEEDS_STAGE_1_DECORATORS) {
     return function(...params) {
       if (isStage2Descriptor(params)) {
         let desc = params[0];
 
-        return fn(desc);
+        return deprecateDirectDescriptorMutation(fn, desc);
       } else {
         let desc = convertStage1ToStage2(params);
 
-        fn(desc);
+        desc = deprecateDirectDescriptorMutation(fn, desc);
 
         if (typeof desc.finisher === 'function') {
           // Finishers are supposed to run at the end of class finalization,
@@ -112,7 +129,7 @@ export function decorator(fn) {
 
         return desc.descriptor;
       }
-    }
+    };
   } else {
     return fn;
   }
@@ -165,10 +182,13 @@ export function decoratorWithParams(fn) {
  */
 export function decoratorWithRequiredParams(fn, name) {
   return function(...params) {
-    assert(`The @${name || fn.name} decorator requires parameters`, !isDescriptor(params) && params.length > 0);
+    assert(
+      `The @${name || fn.name} decorator requires parameters`,
+      !isDescriptor(params) && params.length > 0
+    );
 
     return decorator(desc => {
       return fn(desc, params);
     });
-  }
+  };
 }