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

Ember.computed.oneWay teardown is "resetting" the property in an inner component #17243

Closed
gitKrystan opened this issue Dec 3, 2018 · 9 comments · Fixed by #17487
Closed

Comments

@gitKrystan
Copy link
Contributor

We’ve found a real stumper of a regression in our app that git bisect says is caused by this commit (65c20ab) in 3.5.0-beta.3.

We have two components. A property is passed in to the outer component, where we use computed.oneWay to duplicate the property, then pass that into the inner component, where it is used directly (not aliased or mutated). Our setup is similar to what is shown in this Twiddle, though I wasn’t able to repro the bug on Twiddle. The property is computed once with some default values, then again on didInsertElement after we have done some measuring.

In production only, it appears that the property gets set to the default, then updated to the correct final value…then set back to the original (incorrect) value. This only happens in the inner component, even though we just pass the value in directly from the outer component and do not alias it or mutate it in the inner component.

Adding a no-op observer on the oneWay property appears to “fix” the issue, which makes sense given the PR description for that commit (#16978), though I admittedly don’t understand that code well enough to understand the mechanism.

@wagenet
Copy link
Member

wagenet commented Dec 3, 2018

@bekzod any thoughts about what might be going on here? I notice that you authored the commit that caused us issues. To be clear, I'm not blaming your change, it may just be exposing something strange that we're doing in our app.

@bekzod
Copy link
Contributor

bekzod commented Dec 5, 2018

Will look when I get time for this :)

@bekzod
Copy link
Contributor

bekzod commented Jan 2, 2019

@gitKrystan pocked around, couldn't reproduce can you post repro project here ?

@mupkoo
Copy link

mupkoo commented Jan 3, 2019

I have similar issue with Ember version 3.5.1. I have aliased my model using the alias computed helper and whenever the model changes the property is not updating. If I use the model property instead of the alias, everything is working. This is happening only on production as well. It is working correctly in development and the tests are passing as well.

@bekzod
Copy link
Contributor

bekzod commented Jan 3, 2019

@mupkoo would be nice if you share reproduction repo or make failing test for ember :)

@mupkoo
Copy link

mupkoo commented Jan 3, 2019

I tried to reproduce it in a new project, but I couldn't.


Here is a snippet from my project

import Controller from '@ember/controller';
import { computed } from '@ember/object';
import { alias } from '@ember/object/computed';

export default Controller.extend({
  queryParams: ['page', 'term'],
  page: 1,
  term: '',

  meetings: alias('model'),

  isEmptyStateVisible: computed('meetings.[]', 'term', function () {
    return !this.term && this.meetings.length === 0;
  })
});
{{#unless isEmptyStateVisible}}
  {{ search-input value=term onchange=(action "filter") }}
{{/unless}}

{{#each meetings key="id" as |meeting|}}
  {{ meeting-item meeting=meeting }}
{{/each}}

If I remove the isEmptyStateVisible computed property, that depends on meetings.[], the list gets re-rendered as expected.

@mupkoo
Copy link

mupkoo commented Jan 3, 2019

Changing computed('meetings.[]'... to computed('model.[]'... is fixing the problem as well

@gitKrystan
Copy link
Contributor Author

@chancancode and I are going to look into this either today or Wednesday.

gitKrystan added a commit to gitKrystan/ember.js that referenced this issue Jan 16, 2019
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 added a commit to gitKrystan/ember.js that referenced this issue Jan 17, 2019
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
gitKrystan added a commit to gitKrystan/ember.js that referenced this issue Jan 17, 2019
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>
chancancode added a commit that referenced this issue Jan 18, 2019
@gitKrystan
Copy link
Contributor Author

I deployed master since #17487 was merged, and this issue is fixed in our app. 👌

gitKrystan added a commit to gitKrystan/ember.js that referenced this issue Jan 22, 2019
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>
(cherry picked from commit 1f8218b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants