From 6267e7e90320b40d04d72289291026b09b3bd993 Mon Sep 17 00:00:00 2001 From: Krystan HuffMenne Date: Wed, 16 Jan 2019 14:11:30 -0800 Subject: [PATCH 1/2] Failing test for #17243 Previously #16978 tried to balance the amount of `addDependentKeys` and `removeDependentKeys`. However, it removes the dependent keys to eagerly, causing a different kind of imbalance, and in some cases, this causes the property tags to "miss" updates to aliased properties. Co-authored-by: Godfrey Chan --- .../components/curly-components-test.js | 63 ++++++++++++++++++- .../-internals/metal/tests/alias_test.js | 53 ++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) 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 index 80a864e5418..1a11155d4fc 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js @@ -13,7 +13,7 @@ import { import { run } from '@ember/runloop'; import { DEBUG } from '@glimmer/env'; -import { set, get, observer, on, computed } from '@ember/-internals/metal'; +import { alias, set, get, observer, on, computed } from '@ember/-internals/metal'; import Service, { inject as injectService } from '@ember/service'; import { Object as EmberObject, A as emberA } from '@ember/-internals/runtime'; import { jQueryDisabled } from '@ember/-internals/views'; @@ -3576,6 +3576,67 @@ moduleFor( this.render('{{foo-bar}}'); } + + ['@test ensure aliases are watched properly [GH#17243]']() { + let fooInstance, barInstance; + + let FooComponent = Component.extend({ + source: 'first', + foo: alias('source'), + + init() { + this._super(...arguments); + fooInstance = this; + }, + }); + + this.registerComponent('foo', { + ComponentClass: FooComponent, + template: '{{this.foo}}', + }); + + let BarComponent = Component.extend({ + target: null, + + init() { + this._super(...arguments); + barInstance = this; + }, + + bar: computed('target.foo', function() { + if (this.target) { + return this.target.foo.toUpperCase(); + } + }), + }); + + this.registerComponent('bar', { + ComponentClass: BarComponent, + template: '{{this.bar}}', + }); + + this.render('[][]'); + + this.assertText('[first][]'); + + // addObserver + runTask(() => set(barInstance, 'target', fooInstance)); + + this.assertText('[first][FIRST]'); + + runTask(() => set(fooInstance, 'source', 'second')); + + this.assertText('[second][SECOND]'); + + // removeObserver + runTask(() => set(barInstance, 'target', null)); + + this.assertText('[second][]'); + + runTask(() => set(fooInstance, 'source', 'third')); + + this.assertText('[third][]'); + } } ); diff --git a/packages/@ember/-internals/metal/tests/alias_test.js b/packages/@ember/-internals/metal/tests/alias_test.js index 0546b99ea32..29a7defff42 100644 --- a/packages/@ember/-internals/metal/tests/alias_test.js +++ b/packages/@ember/-internals/metal/tests/alias_test.js @@ -7,6 +7,7 @@ import { addObserver, removeObserver, tagFor, + tagForProperty, } from '..'; import { meta } from '@ember/-internals/meta'; import { moduleFor, AbstractTestCase } from 'internal-test-helpers'; @@ -164,5 +165,57 @@ moduleFor( assert.equal(count, 2); } + + ['@test property tags are bumped when the source changes [GH#17243]'](assert) { + function assertPropertyTagChanged(obj, keyName, callback) { + let tag = tagForProperty(obj, keyName); + let before = tag.value(); + + callback(); + + let after = tag.value(); + + assert.notEqual(after, before, `tagForProperty ${keyName} should change`); + } + + function assertPropertyTagUnchanged(obj, keyName, callback) { + let tag = tagForProperty(obj, keyName); + let before = tag.value(); + + callback(); + + let after = tag.value(); + + assert.equal(after, before, `tagForProperty ${keyName} should not change`); + } + + defineProperty(obj, 'bar', alias('foo.faz')); + + assertPropertyTagUnchanged(obj, 'bar', () => { + assert.equal(get(obj, 'bar'), 'FOO'); + }); + + assertPropertyTagChanged(obj, 'bar', () => { + set(obj, 'foo.faz', 'BAR'); + }); + + assertPropertyTagUnchanged(obj, 'bar', () => { + assert.equal(get(obj, 'bar'), 'BAR'); + }); + + assertPropertyTagUnchanged(obj, 'bar', () => { + // trigger willWatch, then didUnwatch + addObserver(obj, 'bar', incrementCount); + removeObserver(obj, 'bar', incrementCount); + }); + + assertPropertyTagChanged(obj, 'bar', () => { + set(obj, 'foo.faz', 'FOO'); + }); + + assertPropertyTagUnchanged(obj, 'bar', () => { + assert.equal(get(obj, 'bar'), 'FOO'); + }); + } } ); From 1f8218b60e817f6403191473580fc63e072bf151 Mon Sep 17 00:00:00 2001 From: Krystan HuffMenne Date: Wed, 16 Jan 2019 16:01:07 -0800 Subject: [PATCH 2/2] Don't remove dep keys in `didUnwatch` This fixes #17243 by only removing dependent keys in `teardown`. Since #16978 coalesces the work to add dependent keys to happen at most once, it is now incorrect to eagerly remove them in `didUnwatch` because that could happen for a variety of reasons while there are still other interested parties (see attached test cases). This limits the work of removing dependent keys to `teardown` only. The logic is that, while we want to defer setting up the "link" to the target property as lazily as possible, it is less important to "unlink" it eagerly once the work is done. It _may_ be possible to accomplish this, but the current amount of book-keeping is not sufficient to track the count properly. In our tests, we have created sequences like this: 1. setup (peekWatching = 0, so nothing happens) 2. get (consumed here) 3. willWatch (already consumed, no-op) 4. get (already consumed, no-op) 5. didUnwatch 6. ... In this case, it would be incorrect to "unlink" at step 5. As of PR #16978, `CONSUMED` is essentially a boolean flag, so it is not sufficient to track the balance, and also, there is no counterpart to `get`, which makes eager "unlinking" impossible without much more book-keeping. It's unclear that it would be worthwhile to do that. On the other hand, if we only "unlink" on teardown, this ensures that we are only "unlinking" at most once, and it is guaranteed that there are no longer any interested parties. Fixes #17243 Co-authored-by: Godfrey Chan --- packages/@ember/-internals/metal/lib/alias.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/alias.ts b/packages/@ember/-internals/metal/lib/alias.ts index 38471c23882..b8f3b6cdb1e 100644 --- a/packages/@ember/-internals/metal/lib/alias.ts +++ b/packages/@ember/-internals/metal/lib/alias.ts @@ -46,10 +46,6 @@ export class AliasedProperty extends Descriptor implements DescriptorWithDepende this.consume(obj, keyName, meta); } - didUnwatch(obj: object, keyName: string, meta: Meta): void { - this.unconsume(obj, keyName, meta); - } - get(obj: object, keyName: string): any { let ret = get(obj, this.altKey); this.consume(obj, keyName, metaFor(obj));