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

Adds types to FetchService #45

Merged
merged 13 commits into from
Mar 22, 2023
8 changes: 4 additions & 4 deletions web/app/components/inputs/people-select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { tracked } from "@glimmer/tracking";
import { inject as service } from "@ember/service";
import { task } from "ember-concurrency";
import { action } from "@ember/object";
import FetchService from "hermes/services/fetch";
import { assert } from "@ember/debug";

export interface Person {
emailAddresses: { value: string }[];
Expand All @@ -18,9 +20,7 @@ interface PeopleSelectComponentSignature {
}

export default class PeopleSelectComponent extends Component<PeopleSelectComponentSignature> {
// @ts-ignore
// FetchService not yet in the registry
@service("fetch") declare fetchSvc: any;
@service("fetch") declare fetchSvc: FetchService;

/**
* The list of people to display in the dropdown.
Expand Down Expand Up @@ -66,7 +66,7 @@ export default class PeopleSelectComponent extends Component<PeopleSelectCompone
}),
});

const peopleJson = await res.json();
const peopleJson = await res?.json();

if (peopleJson) {
this.people = peopleJson.map((p: Person) => {
Expand Down
5 changes: 4 additions & 1 deletion web/app/routes/authenticated/drafts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

return response.json();
});
return response;
} catch (e: unknown) {
console.error(e);
Expand Down
5 changes: 4 additions & 1 deletion web/app/routes/authenticated/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ConfigService from "hermes/services/config";
import AlgoliaService from "hermes/services/algolia";
import FetchService from "hermes/services/fetch";
import AuthenticatedUserService from "hermes/services/authenticated-user";
import { assert } from "@ember/debug";

export default class SettingsRoute extends Route {
@service("config") declare configSvc: ConfigService;
Expand All @@ -15,7 +16,9 @@ export default class SettingsRoute extends Route {
const allProducts = await this.fetchSvc
.fetch("/api/v1/products")
.then((resp) => {
return resp.json()})
assert("response must be defined", resp);
return resp.json();
})
.catch((err) => {
console.log(`Error requesting products: ${err}`);
});
Expand Down
2 changes: 2 additions & 0 deletions web/app/services/authenticated-user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export default class AuthenticatedUserService extends Service {
this.session.data.authenticated.access_token,
},
});

assert('response must be defined', response)
let subscriptions: string[] = await response.json();

let newSubscriptions: Subscription[] = [];
Expand Down
16 changes: 11 additions & 5 deletions web/app/services/fetch.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
// @ts-nocheck
// TODO: Type this file.
import Service from "@ember/service";
import fetch from "fetch";
import { inject as service } from "@ember/service";
import SessionService from "./session";

interface FetchOptions {
method?: string;
headers?: {
[key: string]: string;
};
body?: string;
}

export default class FetchService extends Service {
@service session;
@service declare session: SessionService;

async fetch(url, options = {}) {
async fetch(url: string, options: FetchOptions = {}) {
// Add the Google access token in a header (for auth) if the URL starts with
// a frontslash, which will only target the application backend.
if (Array.from(url)[0] == "/") {
Expand All @@ -18,7 +25,6 @@ export default class FetchService extends Service {
};
}
try {

const resp = await fetch(url, options);

if (!resp.ok) {
Expand Down
3 changes: 2 additions & 1 deletion web/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
"hermes/tests/*": ["tests/*"],
"hermes/*": ["app/*"],
"hermes/mirage/*": ["mirage/*"],
"*": ["types/*"]
"*": ["types/*"],
"fetch": ["node_modules/ember-fetch"]
}
},
"include": ["app/**/*", "tests/**/*", "types/**/*"]
Expand Down