[3.13] Regression in computed property computation

[boris-petrov]

Here is a reproduction repo

The last commit updates Ember to 3.13. When typing in the input, nothing happens.

The previous commit uses 3.12 and there, when typing in the checkbox, in the console a number of things happen - the computed property's get is called and the value is also propagated to the "parent" component/controller.

This is a bug/regression.

[rwjblue]

the computed property's get is called and the value is also propagated to the "parent" component/controller

Why does this happen!?! This seems completely opposite of how the system is supposed to work (to my knowledge).

[boris-petrov]

Do you mean that it wasn’t supposed to work before too? How would you do something similar?

[rwjblue]

Do you mean that it wasn’t supposed to work before too?

Yes, I am surprised this ever mutated the parent properties value (to the point where I would consider the fact that it did a bug itself). In addition, it seems to me like it should never have recomputed at all (since the value is not consumed).

How would you do something similar?

From the demo repo it is very hard to tell what you actually want to do here. Can you explain in prose what the goal is (aka if you had to write a "user story" for this in your app, what would it say)?

[boris-petrov]

@rwjblue - thanks for the support.

1. Why wouldn't it recompute? In the application template this.foo is logged. It is gotten from and set into the Foo component. There is an input in that component that changes some inner value on which the foo property depends on. I'm not sure why it shouldn't work, could you explain?

2. Our use case is very similar to this code. We have a "global" query string property in the application template which we pass to a global-search component (which actually sets that property for it to be used by the application controller). In the global-search component there are some details but in the end the code is very similar - we construct the "query" by combining some strings which come from different inputs - hence the this.parts.mapBy('value').join(''). Not sure if I explain it very well. But I still am not sure why it shouldn't work the way we've written it (even though it's not the pretties code for sure).

[rwjblue]

Why wouldn't it recompute?

Because nothing uses or sets the value of foo. In the application template, the value that is used is the application controllers own value for foo. Nothing in the Foo component mutates the argument at all.

There is an input in that component that changes some inner value on which the foo property depends on.

Sure, I agree but nothing actually uses the computed property at all in the component. This is effectively an observer (you want to mutate upwards when the parts value changes), if you swap to use an observer (and have the observer this.set('foo', newValue)) it will almost certainly work.

[boris-petrov]

OK, I almost agree that the observed behavior on 3.13 is correct, but:

1. Even if I put a {{log 111 this.foo}} in foo.hbs - then the computed property is gotten and that log prints, but the log in application.hbs still doesn't show up. Which means that this value is not propagated back to the parent controller/component. This, surely, is a bug?

2. We actually do use the setter of foo - that is, we pass a value from the parent controller for foo and also expect something back. If we refactor away the computed property (because as you say it is not directly used as-is in the component) we'll have to have two observers in the Foo component or different values to be used for the upward and downward values which will complicate the code perhaps. I'll have to try to see how it works out but I believe that the way right now is the "simplest".

[Kilowhisky]

Just to throw my info on this thread, i think i encountered this same regression in a different way.
I'm not saying that this needs to be corrected as i can see how what i concocted is convoluted and has a usable work around. But i still think it should be noted.

I think i have a problem attaching two-way binding to a computed property with the get(),set() pattern.

I have a component that has a two-way bound property.
{{ input-latlng decimal=latDecimal }}

This component displays input boxes based on the users preferred latitude format (degrees minutes seconds or degrees decimal minutes or .... ).

Internally this component takes that decimal value and creates computed properties based with each sub format component with a main internal value as the 'backer' for the units. These individual computed properties are then shown to the user as various input boxes.

When a user types into one of those boxes the property value is set to the backer unit and this then propagates to the other computed properties in the component this is then propagated up the chain to the decimal value and then to the parent component.

This is the chain:

1. Component declares input decimal value.
2. Component set() applies decimal to backing base value.
3. All other computed properties update as they are watching that value.
4. User alters one of those properties.
5. That property set() applies changes to backing base value.
6. All other computed properties update as they are watching that value. This means that the decimal value is recomputed.
7. Two-way binding takes the change to the parent as the decimal get() has generated a new value.

So this is where the problem lies. Step 7 does not happen in 3.13.
Again, this is probably an anti-pattern these days with DDAU style. Its just a really hard problem to identify without checking every single property.

[alexlafroscia]

I think I may be experiencing the same problem, but expressing itself a different way.

I have a computed property like this:

value: computed({
  get() {
    return null;
  },
  set(value) {
    // Do some side effect
    return value;
  }
});

This is bound to an input

{{input value=value}}

This works fine on Ember 3.12.

On Ember 3.13 (and 3.14, and 3.15) the getter is re-computed each time, rather than using the cached value from the setter, which was the previous behavior.

The resulting UX is that no matter how much you type into the input field, value is always null and you therefore never actually get anything in the input field.

I recognize that this is an anti-pattern and that this code should adhere to DDAU, but this is old code that has not been updated yet. While I can refactor this to use DDAU, the reality is that this pattern is all over our codebase and the previous behavior should not have broken.

[pzuraq]

Can you confirm that this is still an issue in 3.16 @alexlafroscia? Lot's of fixes have landed for tracked properties since the last release. If it is still an issue, we definitely should (and will) fix it and land it in 3.16, since that is the LTS.

[alexlafroscia]

Yes; the issue that I am seeing on Ember 3.13-3.15 is also an issue on 3.16.

Just to make sure I'm not going crazy, I went back to 3.12 and made sure to verify that that version does not have this problem (and it does not).

To clarify, my understanding is that a computed property with a setter will use the return value of the setter from that point on and not re-evaluate the getter. To my understanding, that's how Ember versions up to and including 3.12 have worked.

[pzuraq]

@alexlafroscia I'm unable to reproduce this in Ember 3.15 or 3.16: https://glitch.com/edit/#!/coffee-plywood-1ks15ajvx4?path=app/templates/application.hbs:4:0

Can you provide a reproduction, via Glitch or other code samples?

[alexlafroscia]

Very odd... I'll have to keep digging into my app to see what the difference here is.

Also -- TIL you can use Ember in Glitch. Neat!

[alexlafroscia]

Reproduction on Glitch here:

https://glitch.com/~cherry-bovid-ie855acxrz

On Ember 3.12, updating value on the inside of the component is two-way bound to the variable on the outside of the component (outerValue on the Application controller).

On Ember 3.15, value updated inside the component does not update the variable outside of the component. It seems like the empty value on the outside overwrites the value on the inside of the component each time it is updated.

Removing the computed property resolves the issue.

[richgt]

I've got a github repo that reproduces what I think is the major issue here: https://github.com/richgt/ember-computed-issue. If you change the Ember Version to 3.12, the "String" updates as you change the "A" and "B" inputs. In 3.13+, it does not.

[richgt]

I've been talking with @rwjblue about this. Here's a PR that adds a test to the Ember Codebase documenting the failure: #19028.

Per Robert, he's located the cause, working on a solution.

in 3.12, this is what happens:

1. Ember.set(child, 'a', 'foo')
2. Calling Ember.set calls notifyPropertyChange(child, 'a')
3. notifyPropertyChange calls dependentKeysDidChange
4. dependentKeysDidChange ultimately causes notifyPropertyChange(child, 'value') to be called
5. calling notifyPropertyChange for a value that was passed in to a component calls that components PROPERTY_DID_CHANGE method
6. Ember.Component.prototype[PROPERTY_DID_CHANGE] implements the two way binding behaviors, and ultimately calls Ember.set(parent, 'string', 'foo')
   In 3.13+ there is no such thing as dependentKeysDidChange
   IF we had called notifyPropertyChange(child, 'value') the parent's property would have been mutated properly, but we just don't call it (since no code exists to do it)

[boris-petrov]

@rwjblue, @Kilowhisky, @alexlafroscia, @pzuraq, @richgt - I think this has since been fixed. At least in 3.25.1 the original repo that I've given works fine again. Can you all confirm that your use-cases have been fixed also so we can close the issue?

@rwjblue - you mentioned that this should not happen - is that still your opinion? Because in 3.25.1 it again works as I expect it and as you expect it not to. 😄

[richgt]

Yes, I believe @rwjblue and @pzuraq fixed it in #19028.