-
Notifications
You must be signed in to change notification settings - Fork 96
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
Adds types to FetchService #45
Conversation
1741fa5
to
acc1131
Compare
acc1131
to
38447cb
Compare
@@ -93,7 +93,10 @@ export default class DraftsRoute extends Route { | |||
"/api/v1/drafts?" + | |||
this.createDraftURLSearchParams(params, ownerFacetOnly) | |||
) | |||
.then((response) => response.json()); | |||
.then((response) => { | |||
assert("response must be defined", response); |
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'm curious about the uses of assert
in this PR because assertions are removed for production builds - can you explain the intent behind these?
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.
To pass type checking and hopefully to pinpoint errors better in development. resp
can be undefined so we have to assert it or invoke it with a question mark resp?.json()
. I expect a response so I opted to assert it.
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.
Thinking about this a little more. I see where you're coming from. It's both more succinct and more true to use the resp?.json()
invocation. Any benefit of assertion-error proximity is minimal. I'm going to change the approach!
* Adds types to FetchService * Update maybeUndefined * Add fetch to tsconfig * Fix type errors * Update people-select.ts * Remove unnecessary import
Adds types to the FetchService