Diff @actions/http-client changes #1060
Conversation
* Revert "Exec 1.2.0 patch" c9f7927 * glob 0.3.0 release
feat: @actions/core extensions for markdown summary
@actions/core 1.7.0 release
| @@ -3,7 +3,7 @@ import * as net from 'net' | |||
| import * as core from '@actions/core' | |||
| import * as configVariables from '../src/internal/config-variables' | |||
| import {retry} from '../src/internal/requestUtils' | |||
| import {IHttpClientResponse} from '@actions/http-client/interfaces' | |||
| import {IHttpClientResponse} from '@actions/http-client/lib/interfaces' | |||
There was a problem hiding this comment.
Having to put lib in the module reference seems to be a lerna thing. I tested what happens if you install the actual package through npm and it doesn't change.
Other packages in this repo reference each other's submodules through lib/.
| @@ -39,7 +39,7 @@ describe('basics', () => { | |||
| // "url": "https://httpbin.org/get" | |||
| // } | |||
|
|
|||
| it('does basic http get request', async done => { | |||
| it('does basic http get request', async () => { | |||
There was a problem hiding this comment.
Got an error about passing done to an async callback. The Jest version in this project is a couple major versions newer than what http-client had, so I figured that was a breaking change that went in.
| @@ -1,5 +1,8 @@ | |||
| # @actions/core Releases | |||
|
|
|||
| ### 1.7.0 | |||
There was a problem hiding this comment.
might want to rebase to smooth out this diff
There was a problem hiding this comment.
I tried that and it made this change go away, but for some reason that made the files in http-client/src all show up as "added" instead of "modified" (????). Since those are the files that really matter here, I switched it back to the pre-rebase commit.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@actions/core", | |||
| "version": "1.6.0", | |||
packages/artifact/package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@actions/artifact", | |||
| "version": "1.0.0", | |||
| "version": "1.0.1", | |||
| "preview": true, | |||
There was a problem hiding this comment.
We should update all the package-lock.json as well, alongside release notes
There was a problem hiding this comment.
👍 I think CI is failing because I need to update the package-lock.jsons. I'm struggling a bit to figure out how to get npm to update them with lerna in the mix
packages/http-client/src/auth.ts
Outdated
| ): Promise<ifm.IHttpClientResponse> { | ||
| return null | ||
| throw new Error("not implemented") |
There was a problem hiding this comment.
Returning null fails type checking with strict: true. I could have just added | null to the return type, but that doesn't make sense for when the method is actually implemented. So better to throw an exception for when it's not implemented. Callers can use canHandleAuthentication() to make sure the method is implemented.
| } else { | ||
| proxyVar = process.env['http_proxy'] || process.env['HTTP_PROXY'] | ||
| } | ||
| const proxyVar = (() => { |
There was a problem hiding this comment.
Why turn this into a function, shouldn't we set the string? No need to invoke this multiple times
There was a problem hiding this comment.
It's not a function, it's still a string. The RHS of the assignment is an IIFE. I did that to use const and to avoid using undefined.
| @@ -1,37 +1,45 @@ | |||
| { | |||
| "name": "@actions/http-client", | |||
| "version": "1.0.11", | |||
| "version": "2.0.0", | |||
There was a problem hiding this comment.
can we add/update the releases.md with information on why we have published a new major version, update the readme, and let users know why they should use it?
brcrista
force-pushed
the
brcrista/http-client-base
branch
2 times, most recently
from
bc486b2
to
e1b5123
Compare
brcrista
force-pushed
the
brcrista/http-client
branch
from
ec32a39
to
fc57cec
Compare
|
This is out-of-date now. I'll open a new one |
|
Can you please show me how to retrieve my Ethereum off of Etherscan?
Micdeprosse21
On Wed, Apr 27, 2022 at 6:52 PM, Brian ***@***.***> wrote:
Closed #1060.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
Note: the base branch for this PR is
brcrista/http-client-base, 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-clientbuilding along with toolkit. See #1062 for the actual PR to the mainline.The biggest change is having to update some exported types in
http-clientto 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.