-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Diff @actions/http-client changes #1064
Conversation
fix failing http-client tests npm run lint-fix Simple manual fixes for lint rules Silence some linter errors Silence some linter warnings, but not "explicit any" Silence @typescript-eslint/no-explicit-any fix @typescript-eslint/no-non-null-assertion fix npm audit for http-client don't use I- prefix on interface names Get rid of `any` type in `Headers` Add http-client to the main README Update release notes
ff7f186
to
f300e2c
Compare
let response: HttpClientResponse | ||
while (numTries < maxTries) { | ||
let response: HttpClientResponse | undefined | ||
do { |
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.
Switched to a do-while loop so the compiler knows we don't return undefined
. In the previous logic, the loop should always execute at least once, but technically it could execute 0 times if someone passed a RequestOptions
with maxRetries
set to a negative number.
export interface IHeaders { | ||
[key: string]: any | ||
export interface Headers { | ||
[header: string]: string |
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 looked at existing usage in http-client
and other toolkit packages and we always use string values for headers. Node's http
module tends to use string | string[] | undefined
for header values, but that would have required more code changes to handle where headers are consumed in http-client
.
canHandleAuthentication(response: ifm.IHttpClientResponse): boolean { | ||
canHandleAuthentication(): boolean { |
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.
The linter errors on unused parameters. TypeScript still considers this as implementing the interface method.
import https = require('https') | ||
import ifm = require('./interfaces') | ||
import pm = require('./proxy') | ||
/* eslint-disable @typescript-eslint/no-explicit-any */ |
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.
There were a lot of any
's in this file and I didn't want to spend time eliminating all of them if it's already tested.
export interface IHeaders { | ||
[key: string]: any | ||
} |
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.
Deleting this in favor of the standard http.OutgoingHttpHeaders
.
export interface IHttpClientResponse { | ||
message: http.IncomingMessage | ||
readBody(): Promise<string> |
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.
We already have the HttpClientResponse
class. We were using this interface for creating mocks in testing. You don't need that in TypeScript because of duck typing.
💡 See #1064 for a better diff! https://github.com/actions/toolkit contains a variety of packages used for building actions. https://github.com/actions/http-client is one such package, but lives outside of the toolkit. Moving it inside of the toolkit will improve discoverability and reduce the number of repos we have to keep track of for maintenance tasks (such as github/c2c-actions-service#2937). I checked with @bryanmacfarlane on the historical decision here. Apparently it was just inertia from before we released the toolkit as multiple packages. The benefits here are: - Have one fewer repo to keep track of - Signal that this is an HTTP client meant for building actions, not for general use. ## Notes - `@actions/http-client` will continue to be released as its own package. - Bumping the package version to **2.0.0**. Since we're compiling in strict mode now, there are some breaking changes to the exported types. This is an improvement because the null-unsafe version of`http-client` is currently breaking the safety of null-safe consumers. - I'm not updating the other packages to use the new version in this PR. I plan to do that in a follow-up. We'll hold off on publishing `http-client` v2 to NPM until that's done just in case other changes shake out of it.
💡 See actions/toolkit#1064 for a better diff! https://github.com/actions/toolkit contains a variety of packages used for building actions. https://github.com/actions/http-client is one such package, but lives outside of the toolkit. Moving it inside of the toolkit will improve discoverability and reduce the number of repos we have to keep track of for maintenance tasks (such as github/c2c-actions-service#2937). I checked with @bryanmacfarlane on the historical decision here. Apparently it was just inertia from before we released the toolkit as multiple packages. The benefits here are: - Have one fewer repo to keep track of - Signal that this is an HTTP client meant for building actions, not for general use. ## Notes - `@actions/http-client` will continue to be released as its own package. - Bumping the package version to **2.0.0**. Since we're compiling in strict mode now, there are some breaking changes to the exported types. This is an improvement because the null-unsafe version of`http-client` is currently breaking the safety of null-safe consumers. - I'm not updating the other packages to use the new version in this PR. I plan to do that in a follow-up. We'll hold off on publishing `http-client` v2 to NPM until that's done just in case other changes shake out of it.
Note: the base branch for this PR is
brcrista/http-client-diff
, notmain
. This is a branch I created just from copying over the files from https://github.com/actions/http-client without any changes.The purpose of this PR is to highlight the changes I had to make to get
@actions/http-client
building along with toolkit. See #1062 for the actual PR to the mainline.The biggest change is having to update some exported types in
http-client
to get it to build withstrict
. For example, null checking is now enforced. While there are more involved ways to clean this up (for example, to eliminate the possibility of returning nulls), I tried just to describe the current state of things without making any changes to behavior.There are also many minor changes to satisfy linting rules.