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

chore: Refactor types for fetching GitHub content #217

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Conversation

kemmerle
Copy link
Contributor

@kemmerle kemmerle commented Dec 4, 2024

Description and Context

Is a dependency of: HubSpot/hubspot-cli#1288

As part of our work to convert the CLI to TypeScript, we're typing prompts that rely on downloading content from GitHub. We were mistakenly returning the Buffer type from the fetchFileFromRepository function. In fact, this function returns the contents of the file (resp.data) we're downloading. Since literally anything can be in a file downloaded from GitHub, this is an instance of a true any type, in my opinion.

TODO

  • Address feedback.

Who to Notify

@camden11 @brandenrodgers @joe-yeager

Copy link
Contributor

@camden11 camden11 left a comment

Choose a reason for hiding this comment

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

Hmm I can see how Buffer is causing some issues but I think we could come up with a more type safe approach here. First, Buffer is technically correct since we're fetching a file from the endpoint, but it seems like when you fetch a json file, Axios knows how to handle it and treats it like a normal endpoint response. I think the move here is probably to use a generic type so we can tell the fetchFileFromRepository what to expect if we know it's not a Buffer. We should also extend this to the fetchRepoFile function in api/github

export function fetchRepoFile<T = Buffer>(
  repoPath: RepoPath,
  filePath: string,
  ref: string
): HubSpotPromise<T> {
  return axios.get<T>(
    `${GITHUB_RAW_CONTENT_API_PATH}/${repoPath}/${ref}/${filePath}`,
    {
      headers: {
        ...getDefaultUserAgentHeader(),
        ...getAdditionalHeaders(),
      },
    }
  );
}

and then you can add the generic type to fetchFileFromRepository as well

@kemmerle kemmerle requested a review from camden11 December 4, 2024 18:59
lib/github.ts Outdated Show resolved Hide resolved
@kemmerle kemmerle merged commit 3593bb9 into main Dec 5, 2024
1 check passed
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.

2 participants