Additional test coverage for async belongsTo mutation

[jlami]

Test for #5575

[runspired]

Unfortunately, this test does not test the issue reported in #5575. We don't expect the proxy instance to have changed, only the value proxied. The proxy instance should be stable.

I produced a test ensuring that the value does indeed update, and we can see that it does (commit pushed).

[jlami]

I believe your test works with sync belongsTo, while my problem was with async. But I don't know if this makes any difference.

[runspired]

@jlami you can see here that I am working with an async belongsTo: https://github.com/emberjs/data/pull/5584/files#diff-6c89bc797aa543dc33556580b5705bedR22

Your test was assuming that the instance of the PromiseProxy would change every time, which is not a safe assumption. The instance will be stable, but it's content and promise will change.

[jlami]

@jlami you can see here that I am working with an async belongsTo: https://github.com/emberjs/data/pull/5584/files#diff-6c89bc797aa543dc33556580b5705bedR22

ok, but your asserts are on the other side of the relationship, which is sync

Your test was assuming that the instance of the PromiseProxy would change every time, which is not a safe assumption. The instance will be stable, but it's content and promise will change.

Yes, I'm assuming that the problem in #5575 is that the belongsTo should change value. But I think that might be better discussed over there.

[runspired]

ok, but your asserts are on the other side of the relationship, which is sync

they are on both sides of the relationship

I'm assuming that the problem in #5575 is that the belongsTo should change value.

the PromiseProxy does change value, both the content and the promise are updated, and usually this causes anything watching the proxy to recalculate. This makes me suspect a timing issue is causing this to not be true, which typically means that an observer is present. Your pointing out the following also supports the observer problem theory:

BTW it works after the first choice because removeInternalModelFromOwn resets the _promiseProxy to null. But since in the bug we start with null as belongsTo removeInternalModelFromOwn is never called.

The bug that I think the above is causing is that we notify both for the removal and for the addition and then again once we've settled state. This is a known issue we've had for multiple major versions that I've been hoping to refactor and fix. TL;DR since observers are sync, notifying causes them to trigger instantly. Since state is not settled, they get broken/partial/wrong state when they trigger the call to getData. These problems have surfaced in larger volume post a recent refactor that cleaned up and clarified relationship state, because previously we were less precise about the state we were in and the "fudging" was probably preventing observers from blowing up the world in places they should have been.

What we should do is move to a queue system where we only notify the UI a single time, and only do so once we've settled the state of the relationship.

[jlami]

they are on both sides of the relationship

Ah, yes, I missed the await part.

What we should do is move to a queue system where we only notify the UI a single time, and only do so once we've settled the state of the relationship.

I do agree that the observers do have some problems now with ED and it would be good to improve that. But I still think the bug I'm talking about is not related. I have changed the twiddle to show more clearly what I think is the cause. The didUpdateAttrs with 2 components deep.

https://ember-twiddle.com/8699e6c887c9eb2cb4344fb1d2551dbe?openFiles=controllers.application.js%2C

I have changed the application controller to simulate what ED is doing with the proxy as far as I understand. This illustrates the problems that arise from keeping a reference constant, while using notifyPropertyChange to signal changes. This works one level deep. But the second component will not see it.

[broerse]

@runspired This new constant reference behavior seems to me like a breaking change that could only be done in a ember-data v4.0.0 change. Until then perhaps keep the old behavior or update to ember-data v4.0.0 now.

[runspired]

@broerse it was constant in most situations before too, but more than that, it would defeat the purpose of being a proxy to rely on it being unstable.

There may be a bug in ember-data where we don't notify properly, and given a test showing this behavior I'm happy to poke more at it. However, given the example above, I suspect that one or more of the following statements regarding ember-power-select and this issue is true:

• Ember has a bug in which dirtied proxies are not considered dirty from the perspective of the diff check that determines whether to run didUpdateAttrs and didReceiveAttrs
• Ember has a bug in which leaf dirtiness is just not considered for didUpdateAttrs and didReceiveAttrs
• didUpdateAttrs should be avoided and willRender should be used
• ember-data isn't actually notifying the proxy although we think it is

I'll leave it to folks like @krisselden and @rwjblue to weigh in on those first three; however, as regards the last I'm inclined to think we have no issue given that the rendering tests I added in this PR pass.

Power Select Issue for reference: cibernox/ember-power-select#1147

[broerse]

@runspired @krisselden @rwjblue I don't think this change is necessary wrong but it is breaking and probably Ember/Glimmer needs to be changed too. It is not only power select but all component that check didUpdateAttrs 2 components deep. It basically makes the didUpdateAttrs unuseable in addons and I consider that breaking. See the failing twiddle: https://ember-twiddle.com/8699e6c887c9eb2cb4344fb1d2551dbe?openFiles=controllers.application.js%2C