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

Better types for HttpClient #68

Merged
merged 3 commits into from
Jul 29, 2023
Merged

Better types for HttpClient #68

merged 3 commits into from
Jul 29, 2023

Conversation

bhollis
Copy link
Contributor

@bhollis bhollis commented Jul 28, 2023

  1. Provide a type parameter to HttpClient so that it returns the correct return type instead of any.
  2. Convert all query params to strings and omit undefined values, and give them the type Record<string, string> so they don't need to have anything done to them to be able to be passed into UrlSearchParams.

This will help nail some of the last any usage in the DIM codebase. Major version bump because the types are changing in a way that'll require some code fixes for consumers.

@bhollis bhollis requested a review from nev-r July 28, 2023 07:25
Plus convert all query params to strings and omit undefined values
@robojumper
Copy link
Member

2 looks good. For 1 I'm not sure I understand how consumers are expected to provide a differently typed HttpClient to each endpoint function individually without resorting to any though.

@bhollis
Copy link
Contributor Author

bhollis commented Jul 28, 2023

Ah, yes. I didn't quite understand the difference in practice between HttpClient being a generic type, vs. being a non-generic type that references a generic function.

@bhollis
Copy link
Contributor Author

bhollis commented Jul 28, 2023

OK, switched it up. This lines up with a working change on the DIM side.

@bhollis bhollis merged commit a2aef6e into master Jul 29, 2023
1 check passed
@bhollis bhollis deleted the better-httpclient-types branch July 29, 2023 18:27
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