-
Notifications
You must be signed in to change notification settings - Fork 2
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
Convert to TypeScript (2022 Edition) #1
base: main
Are you sure you want to change the base?
Conversation
da08191
to
708b1fe
Compare
@service declare store: Store; | ||
|
||
async model(): Promise<Array<RentalModel>> { | ||
return (await this.store.findAll('rental')).toArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively
import type ArrayProxy from '@ember/array/proxy';
// ...
async model(): Promise<ArrayProxy<RentalModel>> {
return store.query('rental');
}
This shows users how to type promise-ified methods resolving to array-proxies (RecordArray is just an ArrayProxy) while also avoiding a common pitfall (use of findAll
). The trouble with findAll
is that you must either configure your adapter to eagerly make new fetch requests with each usage, or pass in the options to force it to reload on every call-site, otherwise it will only return what is in the store. This means that if any other request ever returned a rental
or if prior to this call someone had called store.createRecord('rental', {})
then their request would not hit network and not result in the expected data.
Once you've called toArray
on a findAll you've already negated the benefits of having a live view of all records of the type in the store. If that live view was desired, then using query and returning store.peekAll will achieve that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I linked to this comment in the blog post, but I won't be changing the code since I don't want to stray too far from the behavior in the original Super Rentals tutorial. With that said, should we consider changing the behavior there? I was unaware of the differences you mentioned above and I've read the docs pretty thoroughly.
Also, if anyone is following along, the code that actually works is:
async model(): Promise<ArrayProxy<RentalModel>> {
return this.store.query('rental', {});
}
Without the {}
, the code doesn't pass type-checking and throws this error in runtime: Error: Assertion Failed: You need to pass a query hash to the store's query method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that said, should we consider changing the behavior there? I was unaware of the differences you mentioned above and I've read the docs pretty thoroughly.
I definitely think it should be considered. findAll
is one of the "bad parts" of ember-data I often find myself needing to steer folks away from due to how complicated the matrix can be for whether it fires a request or not.
The downside of query
is that repeat calls will always hit network unless you opt to put your own "query cache" in front of it. This is something we're likely going to change this year in ember-data via RFC, as most every app I've seen has written the same "query cache" logic to account for this.
This is the source for my tutorial Converting Your Ember App to TypeScript [2022 Update], part of my Typed Ember extends Confidence series. Visit the tutorial post to follow along.