add test + fix for chained async has many

[sly7-7]

cc/ @Turbo87 Could you confirm your scenario is the same as this one ?

resolves #7684 (at least the mentioned test suite passes locally)

[sly7-7]

Sorry for the noise, this one was just a refactor, getting rid of run().

[igorT]

No apology necessary, thanks a bunch for the cleanup?

[sly7-7]

@Turbo87 At least the "fix" seem to make crates.io's tests happy ;)

[Turbo87]

why do we use polymorphic: true here? at least in crates.io we don't seem to have any polymorphic relationships 🤔

[sly7-7]

just because this is how the relation is defined at the top of the file. (

data/packages/-ember-data/tests/integration/relationships/has-many-test.js

Line 28 in df969d5

messages: hasMany('message', { polymorphic: true, async: false }),

)

I wanted to use the 'same' relation, but async.

[igorT]

@runspired this looks like the correct fix to me, as if you push the same record twice, the deleted branch of code marks the link as not stale so it does not get fetched, even though it had never been fetched. What are your thoughts

[sly7-7]

@igorT Thanks again for looking into it. I've added an assertion to complete the tests (after @runspired pointed me that the state of the hasMany's promise looks not good).
It turns out, that without this "fix" the new assertion pass, but the relationship is not fetched, and with the "fix", the relationship is fetched, but the promise state looks wrong :(

[sly7-7]

As I failed to make the test pass, I tried to make more debugging, to see what happen. I think there are subtle bugs on how the hasMany() computed property is cached/invalidated. For example, considering this code at the end of the added test in rest-adapter-test:

let commentsPromiseArray = post.comments;
let comments = await commentsPromiseArray;
assert.ok(commentsPromiseArray.isFulfilled, 'referenced commentsPromiseArray variable is fulfilled');
assert.equal(post.comments, commentsPromiseArray, 'promise arrays are the same');
assert.ok(commentsPromiseArray.isFulfilled, 'referenced commentsPromiseArray variable is still fulfilled');
assert.ok(post.comments.isFulfilled, 'post.comments promise is fulfilled');
 assert.equal(comments.length, 3, 'The correct records are in the array');

and the "fix" (don't set isStale to false), gives this result:

See how the PromiseManyArray state is reset to not fulfilled just because we call again the post.comments cp.
I think it's because awaiting the promise invalidate the cp cache for the hasMany. In the end internalModel.getHasMany() is called, which call PromiseManyArray._update() which cause its flag to be reset. (Though, I think the underlying adapter.fetchHasMany won't be called a second time, which is expected)

cc/ @igorT

[Turbo87]

@igorT any news on this? I've just hit the same issue in another project too 😅

[benedikt]

We've ran into the issue as well and rolled back to 3.27 for now. If there's anything I can do to help with fixing this, please let me know.

[sly7-7]

@benedikt Same here about sticking with 3.27.

If there's anything I can do to help with fixing this, please let me know.

I would say, if you find how to make the tests pass in this PR, it could be a potential fix for the issue. I really don't know what to do :(

[benedikt]

It looks like post.comments will trigger an update of the promise proxy. That's why this assertion fails. Removing that proxy update fixes this particular test, but obviously breaks a few others.

I guess the question is what we expect to get from consecutive post.comments calls. Are they supposed to return the cached results or do they all trigger a new fetch?

As the relationship is marked as async, I guess the behavior is correct? 🤔

[sly7-7]

As I said in a previous comment I don't know how consecutive calls of post.comments should behave :(. And it looks like that calling post.comments won't always trig the update you mentioned (since DS.hasMany() is a computed property).

[benedikt]

I should have read all the comments before diving into the code 😬 🙈

[benedikt]

Looks like the computed property gets invalidated while loading the data for the first time in the replace related record operation which eventually calls notifyPropertyChange.

The first call loads the data, which changes the state, which invalidates the property. The second time (probably?) also loads the data, but the state doesn't change (because it's the same data) and therefore doesn't invalidate the property.

I don't have a solution, but hopefully that explains why the post.comments call sometimes returns a new PromiseManyArray and sometimes not.

[esbanarango]

👋. We're also blocked by this one. Back to 3.27. Thank you @sly7-7 and @igorT for putting the time to fix this 🙏🙏.

[runspired]

I'm good with the proposed deletion of the isStale = false state update branch

[sly7-7]

@runspired In that case I must remove these checks https://github.com/emberjs/data/pull/7691/files#diff-e129eb0fe00707712a6ceb0f1fc5abbd5b0a90fdd95281f0923d8904ff39abd8R1335 from the tests, because they fail

I'm really not sure what it implies for consuming apps, for example, when using this promise state flag in template. I think it will be wrongly false in some cases.

[runspired]

@sly7-7 I don't think you need to remove the checks. Does need to be investigated as to why it fails, seems the promiseArray is unstable which it likely shouldn't be.

[sly7-7]

@snewcomer Well, for me it's complicated. I think async relationships are somewhat broken since 3.28 (you can see #7684 and also issues linked to this one). With this change, the issue seemed to be fixed, but at the same time it surfaces an other one, where the state of the underlying promise is unstable (eg, calling twice post.comments.isFulfilled, gives different results). And for this, I don't know what should be done. (see #7691 (comment)).

[sly7-7]

Thank you so much @runspired. I would never have guessed the culprit was here. I don't know how you found that. All look so easy for you 😅