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

Use _scheduleFetch instead of _fetchRecord for belongsTo relationship #5671

Merged
merged 2 commits into from Oct 19, 2018

Conversation

mydea
Copy link

@mydea mydea commented Oct 8, 2018

In order to ensure coalesceFindRequests works for this case.

Without this, it would trigger one find-request per belongsTo relationship, instead of correctly coalescing them together.

I have stumbled over this while trying to debug something else, and was pretty puzzled as to why it wasn't working as expected. Changing this fixed the issue for me - not sure if there is something I'm not seeing there, though.

In order to ensure coalesceFindRequests works for this case.
@runspired
Copy link
Contributor

@mydea could you add some tests for coalescing? Both in the default and for relationships that don't have links? (we can consider the links case from our conversation in #5574 once the RFC is complete). Thank you for doing all this work (including the RFC)!

@mydea
Copy link
Author

mydea commented Oct 9, 2018

I added tests for this case - I hope in the right place, I wasn't entirely sure where to put this.
The tests cover fetching 2 belongsTo relationships concurrently, once with coalesceFindRequests and once without, and ensure that they run through either findMany or findRecord.

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the first test, the other tests need similar fixes.

assert.equal(type.modelName, 'tag', 'modelName is tag');

if (id === '3') {
return {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return resolve(...data...) will make this appropriately return a promise.

},
};
} else if (id === '4') {
return {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, return a resolved promise

}
};

return run(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using async ... await instead of lots of run loops would clean this up nicely


env.adapter.coalesceFindRequests = false;

run(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we make this an async test there is no need to wrap store.push in run()

@mydea mydea force-pushed the use-schedule-fetch-for-belongs-to branch from 9c6f59f to bd92a47 Compare October 18, 2018 06:39
@mydea
Copy link
Author

mydea commented Oct 18, 2018

I made the requested changes to the test (to be honest, I only used to non-async-await syntax because I tried to stick to the same test style as the existing tests in the file. async-await is soo much nicer)!

@mydea mydea force-pushed the use-schedule-fetch-for-belongs-to branch from bd92a47 to 10acf7f Compare October 18, 2018 07:04
@runspired runspired merged commit f14ceb8 into emberjs:master Oct 19, 2018
runspired pushed a commit that referenced this pull request Oct 26, 2018
…#5671)

* Use _scheduleFetch instead of _fetchRecord for belongsTo relationship

In order to ensure coalesceFindRequests works for this case.

* Add tests for coalescing of belongsTo relationships
runspired pushed a commit that referenced this pull request Dec 8, 2018
…#5671)

* Use _scheduleFetch instead of _fetchRecord for belongsTo relationship

In order to ensure coalesceFindRequests works for this case.

* Add tests for coalescing of belongsTo relationships
runspired pushed a commit that referenced this pull request Dec 10, 2018
…#5671)

* Use _scheduleFetch instead of _fetchRecord for belongsTo relationship

In order to ensure coalesceFindRequests works for this case.

* Add tests for coalescing of belongsTo relationships
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants