Skip to content

Commit

Permalink
Don't remove dep keys in didUnwatch
Browse files Browse the repository at this point in the history
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 <godfreykfc@gmail.com>
  • Loading branch information
gitKrystan and chancancode committed Jan 17, 2019
1 parent 6267e7e commit 1f8218b
Showing 1 changed file with 0 additions and 4 deletions.
4 changes: 0 additions & 4 deletions packages/@ember/-internals/metal/lib/alias.ts
Expand Up @@ -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));
Expand Down

0 comments on commit 1f8218b

Please sign in to comment.