Ember.js: [3.13] Regression when observing model changes in controller

[boris-petrov]

If you can't understand the issue title, here is a reproduction repo.

The last commit updates Ember to 3.13.1. Run it and open http://localhost:4200/chat. You'll see recomputing model! in the console. If you click the root link (which just changes the route) and then click the browser's back button, you'll see recomputing model! again. If you repeat that multiple times, you'll see the same message.

Checking out the previous commit which uses Ember 3.12 and repeating the same steps produces a different result - the observer is not triggered at all between route changes and the message is not displayed multiple times. I believe this is the correct behavior.

[efx]

Shot in the dark but is this the same as #18225? Or perhaps related.

[boris-petrov]

@efx - it could be related (or perhaps the same), yes. Not sure, I guess someone would have to dig in. But it is a pretty major issue for us, a lot of recomputation happens because of that.

[pzuraq]

So, the issue here stems from the fact that we made Controller#model a tracked property. This causes two issues:

1. Tracked properties don't check to see if the value has changed, they always invalidate. Every time we enter the route, the model() hook is called and the resulting value is set on the controller. In the past, set would check to see if the value had actually changed before calling notifyPropertyChange, which is why the behavior was different.
2. The sync observer that you added which watches the model actually flushes _later_, on the next set to a property. That seems like a bug for sure.

I realize the fact that the behavior changed here and that could be causing issues for your app, but we have documented for some time that observers may fire whenever a property _could_ have changed, not just when it definitely has. In general, the timing semantics of observers should _not_ be considered public API, which the exception of triggering sync vs async (which you can opt into changing).

If model is not tracked, it won't interop at all with tracked properties and autotracking, which would be a very buggy experience for Octane users. My recommendation here would be to ensure that all of your observers are idempotent - that they can run repeatedly with the same inputs and not cause fundamental issues.

[boris-petrov]

@pzuraq - thank you for the support and thorough explanation!

I've pushed another commit in the reproduction repo. It took me a while, please take a look.

Using Ember 3.13, console.log('getting foo'); is printed twice when you switch back from the application route to the chat one. On 3.12 it is printed only once. This is a computed property so it should not do that. I agree that observers should be idempotent (even though sometimes it is difficult to do so) but computed properties should not recompute when their dependencies have not changed I believe. This is the real problem for us because we have computed properties that actually make queries to the backend and that recomputation is a big hit.

This just might be related to another issue that I opened a while ago, although this here started happening since 3.13.

Do you agree this is a bug?

[pzuraq]

I was discussing this earlier with @chancancode and we actually realized we could replace the tracked property with a computed property, and it shouldn't have these issues. This will work as a temporary fix, so we'll update it to do that and lower friction here.

[boris-petrov]

@pzuraq - replacing the tracked property with a computed one sounds like a good solution for now, however did you manage to take a look at the new commit? Don't you think that the doubly-recomputing property is a deeper issue?

[pzuraq]

This is actually by design - tracked properties will always invalidate whenever they have been set, they do not attempt to compare their values, unlike Ember.set. This is implied by the methods for manual invalidation that were spec'd out in the original RFC.

In general, the idea is that getters should be idempotent and, as much as possible, side-effect free. This way, if they rerun for any reason, they shouldn't cause issues. In cases like yours, I think an ember-concurrency like solution would make the most sense, where if the getter reruns it debounces making a new request since another is still in progress. This way the expensive task is self contained, and not reliant on the exact caching mechanism of tracked properties/change tracking in the app in general.

[boris-petrov]

@pzuraq - I always thought that computed properties never recomputed when not needed (i.e. when their dependencies haven't changed). If that's not the case, then in our case nothing can be done - it is not a "problem" per-se that the getters are recomputed and a request is made to the backend, it's just bad for performance reasons - nothing else. We can live with it though if there is nothing that can be done.

In any case - are you going to do the refactoring of the model thing to be a computed property again? If so, is it going to be in a patch-release for 3.13?

[pzuraq]

Yes, we're going to make sure that all existing properties will continue to work as before. This would be a breaking change otherwise, so we'll definitely be releasing a patch release with this bugfix.

[boris-petrov]

@pzuraq - thank you for fixing this. Do you still need the reproduction repo (for writing a test or something) or I can delete it?

[pzuraq]

Feel free to delete, I think it's fine 👍