Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test and fix for #17243 #17487

Merged
merged 2 commits into from Jan 18, 2019

Commits on Jan 16, 2019

  1. Failing test for emberjs#17243

    Previously emberjs#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 <godfreykfc@gmail.com>
    gitKrystan and chancancode committed Jan 16, 2019
    Copy the full SHA
    6267e7e View commit details
    Browse the repository at this point in the history

Commits on Jan 17, 2019

  1. Don't remove dep keys in didUnwatch

    This fixes emberjs#17243 by only removing dependent keys in `teardown`.
    
    Since emberjs#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 emberjs#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 emberjs#17243
    
    Co-authored-by: Godfrey Chan <godfreykfc@gmail.com>
    gitKrystan and chancancode committed Jan 17, 2019
    Copy the full SHA
    1f8218b View commit details
    Browse the repository at this point in the history