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

Expose axios response #167

Merged
merged 18 commits into from
Jul 30, 2024
Merged

Expose axios response #167

merged 18 commits into from
Jul 30, 2024

Conversation

camden11
Copy link
Contributor

@camden11 camden11 commented Jul 26, 2024

Description and Context

This updates the http module to return full Axios response objects, which will make debugging easier for libraries that consume locla-dev-lib. All API modules will also now return the full response object (they've been updated to use the AxiosPromise return type that axios includes. Their previous return values will now be within the data field on the response object.

A Few other things...

  • Uses this as an opportunity to clean up the API modules a bit. Added some return types and removed a lot of unneeded asyncs.
  • Added a testing util to create Axios responses in __utils__/mockAxiosResponse. This lets tests easily create type accurate axios responses and use them for mocked return values. There might be a better way to do this, but figured this would get the job done for now.

TODO

Need to be very careful to make update all uses of API modules in the CLI to make sure nothing breaks. May also be helpful to PR other HubSpot repos that use local-dev-lib, if needed

Who to Notify

@brandenrodgers @kemmerle @joe-yeager

@camden11 camden11 changed the base branch from main to next July 26, 2024 19:49
@camden11 camden11 marked this pull request as ready for review July 26, 2024 19:55
Copy link
Contributor

@joe-yeager joe-yeager left a comment

Choose a reason for hiding this comment

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

LGTM! Just one minor TS question

Comment on lines +279 to +281
type WarnLogsResponse = {
logs: Array<ProjectLog>;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to move this to a types file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm good question. For smaller types that are only used in one file, I generally just keep them in that file. Though thinking from a library perspective it would probably make sense to export all those types.

This is done in a lot of places though so would need a separate PR

api/github.ts Outdated
export async function fetchRepoAsZip(
zipUrl: string
): Promise<AxiosResponse<Buffer>> {
export async function fetchRepoAsZip(zipUrl: string): AxiosPromise<Buffer> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the async from these ones too?

const resp = await _createDeveloperTestAccount(accountId, accountName);
return resp;
const { data } = await _createDeveloperTestAccount(accountId, accountName);
return data;
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this to @joe-yeager, but these utils are such thin wrappers now that I think we should just delete the entire file. It's not really adding much value at all. We could probably delete lib/sandboxes too, since it's also not doing any additional logic in the api call wrappers.

We can do this in another PR though. It's possible Joe already did it in his error handling PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done it yet, but I'll add an issue so we don't lose track of it and do it in a follow up PR.

Copy link
Contributor

@brandenrodgers brandenrodgers left a comment

Choose a reason for hiding this comment

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

Left some small comments, but otherwise lgtm!

@camden11 camden11 merged commit 41a7f40 into next Jul 30, 2024
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.

3 participants