Merge pull request #18857 from brendenpalmer/fix-get-set-bug-master

Pass value through to `PROPERTY_DID_CHANGE` to avoid calling `get` when setting values for computed props
emberjs · Apr 1, 2020 · 730c638 · 730c638
2 parents 5e90b49 + 64f8ce4
commit 730c638
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 18 deletions.
diff --git a/packages/@ember/-internals/glimmer/lib/component.ts b/packages/@ember/-internals/glimmer/lib/component.ts
@@ -778,7 +778,7 @@ const Component = CoreView.extend(
       this._super();
     },
 
-    [PROPERTY_DID_CHANGE](key: string) {
+    [PROPERTY_DID_CHANGE](key: string, value?: unknown) {
       if (this[IS_DISPATCHING_ATTRS]) {
         return;
       }
@@ -787,7 +787,7 @@ const Component = CoreView.extend(
       let reference = args !== undefined ? args[key] : undefined;
 
       if (reference !== undefined && reference[UPDATE_REFERENCED_VALUE] !== undefined) {
-        reference[UPDATE_REFERENCED_VALUE](get(this, key));
+        reference[UPDATE_REFERENCED_VALUE](arguments.length === 2 ? value : get(this, key));
       }
     },
 

diff --git a/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js b/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
@@ -1496,6 +1496,59 @@ moduleFor(
       this.assertText('In layout - someProp: wycats');
     }
 
+    ['@test setting a value for a computed property then later getting the value for that property works'](
+      assert
+    ) {
+      let componentInstance = null;
+
+      this.registerComponent('non-block', {
+        ComponentClass: Component.extend({
+          counter: computed({
+            set(key, value) {
+              return value;
+            },
+          }),
+
+          init() {
+            this._super(...arguments);
+            componentInstance = this;
+          },
+
+          actions: {
+            click() {
+              let currentCounter = this.get('counter');
+
+              assert.equal(currentCounter, 0, 'the current `counter` value is correct');
+
+              let newCounter = currentCounter + 1;
+              this.set('counter', newCounter);
+
+              assert.equal(
+                this.get('counter'),
+                newCounter,
+                "getting the newly set `counter` property works; it's equal to the value we just set and not `undefined`"
+              );
+            },
+          },
+        }),
+        template: `
+          <button {{action "click"}}>foobar</button>
+        `,
+      });
+
+      this.render(`{{non-block counter=counter}}`, {
+        counter: 0,
+      });
+
+      runTask(() => this.$('button').click());
+
+      assert.equal(
+        componentInstance.get('counter'),
+        1,
+        '`counter` incremented on click on the component and is not `undefined`'
+      );
+    }
+
     ['@test this.attrs.foo === attrs.foo === @foo === foo']() {
       this.registerComponent('foo-bar', {
         template: strip`

diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts
@@ -255,7 +255,6 @@ function noop(): void {}
 export class ComputedProperty extends ComputedDescriptor {
   private _volatile = false;
   private _readOnly = false;
-  private _suspended: any = undefined;
   private _hasConfig = false;
 
   _getter?: ComputedPropertyGetter = undefined;
@@ -676,6 +675,7 @@ export class ComputedProperty extends ComputedDescriptor {
 
     try {
       beginPropertyChanges();
+
       ret = this._set(obj, keyName, value);
 
       finishLazyChains(obj, keyName, ret);
@@ -721,17 +721,7 @@ export class ComputedProperty extends ComputedDescriptor {
     return this._setter!.call(obj, keyName, value);
   }
 
-  setWithSuspend(obj: object, keyName: string, value: any): any {
-    let oldSuspended = this._suspended;
-    this._suspended = obj;
-    try {
-      return this._set(obj, keyName, value);
-    } finally {
-      this._suspended = oldSuspended;
-    }
-  }
-
-  _set(obj: object, keyName: string, value: any): any {
+  _set(obj: object, keyName: string, value: unknown): any {
     let cache = getCacheFor(obj);
     let hadCachedValue = cache.has(keyName);
     let cachedValue = cache.get(keyName);
@@ -755,7 +745,7 @@ export class ComputedProperty extends ComputedDescriptor {
 
     cache.set(keyName, ret);
 
-    notifyPropertyChange(obj, keyName, meta);
+    notifyPropertyChange(obj, keyName, meta, value);
 
     return ret;
   }

diff --git a/packages/@ember/-internals/metal/lib/property_events.ts b/packages/@ember/-internals/metal/lib/property_events.ts
@@ -28,12 +28,18 @@ let deferred = 0;
   @for @ember/object
   @param {Object} obj The object with the property that will change
   @param {String} keyName The property key (or path) that will change.
-  @param {Meta} meta The objects meta.
+  @param {Meta} [_meta] The objects meta.
+  @param {unknown} [value] The new value to set for the property
   @return {void}
   @since 3.1.0
   @public
 */
-function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta | null): void {
+function notifyPropertyChange(
+  obj: object,
+  keyName: string,
+  _meta?: Meta | null,
+  value?: unknown
+): void {
   let meta = _meta === undefined ? peekMeta(obj) : _meta;
 
   if (meta !== null && (meta.isInitializing() || meta.isPrototypeMeta(obj))) {
@@ -47,7 +53,14 @@ function notifyPropertyChange(obj: object, keyName: string, _meta?: Meta | null)
   }
 
   if (PROPERTY_DID_CHANGE in obj) {
-    obj[PROPERTY_DID_CHANGE](keyName);
+    // we need to check the arguments length here; there's a check in `PROPERTY_DID_CHANGE`
+    // that checks its arguments length, so we have to explicitly not call this with `value`
+    // if it is not passed to `notifyPropertyChange`
+    if (arguments.length === 4) {
+      obj[PROPERTY_DID_CHANGE](keyName, value);
+    } else {
+      obj[PROPERTY_DID_CHANGE](keyName);
+    }
   }
 }