fix: fix support for Node.js v18's native fetch when making DELETE requests with no body
#354
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| it("DELETE without request body", () => { | ||
| const options = endpoint("DELETE /user/following/{username}", { | ||
| headers: { | ||
| authorization: `token 0000000000000000000000000000000000000001`, |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| method: "DELETE", | ||
| url: "https://api.github.com/user/following/octocat", | ||
| headers: { | ||
| authorization: `token 0000000000000000000000000000000000000001`, |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| it("POST without request body", () => { | ||
| const options = endpoint("POST /widgets", { | ||
| headers: { | ||
| authorization: `token 0000000000000000000000000000000000000001`, |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| method: "POST", | ||
| url: "https://api.github.com/widgets", | ||
| headers: { | ||
| authorization: `token 0000000000000000000000000000000000000001`, |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| it("PATCH without request body", () => { | ||
| const options = endpoint("PATCH /widgets/{id}", { | ||
| headers: { | ||
| authorization: `token 0000000000000000000000000000000000000001`, |
Check failure
Code scanning / CodeQL
Hard-coded credentials
| method: "PATCH", | ||
| url: "https://api.github.com/widgets/my-widget", | ||
| headers: { | ||
| authorization: `token 0000000000000000000000000000000000000001`, |
Check failure
Code scanning / CodeQL
Hard-coded credentials
|
@wolfy1339 Thanks for the approval! I think I'll wait for @gr2m on this one too, just in case he can think of any reason why it's a bad idea. |
There was a problem hiding this comment.
changes look good, the workarounds no longer seem to be necessary, yay!
I discussed with @timrogers to split up the pull request to address octokit/request.js#506 explicitly and remove the body="" workaround in a separate PR. That makes it easier to revert the changes separately in case something breaks despite our tests.
| "user-agent": userAgent, | ||
| }, | ||
| body: "", |
There was a problem hiding this comment.
I confirmed that the "content-length": 0 is no longer needed for the PUT /user/starred/{owner}/{repo} endpoint.
…` requests with no body Currently, `DELETE` requests without a body don't work on the latest Node.js version, v18, because we try to set `content-length: 0` which is not allowed by the native `fetch` implementation which is included in Node. It throws an error with the `UND_ERR_REQ_CONTENT_LENGTH_MISMATCH` code. This stops manually setting that `content-length: 0` header for `DELETE` requests. I've tested this against the GitHub API with Node.js v18 and v17, and both work as expected, with the GitHub API also accepting the request. As part of this PR, I've also added some new tests covering what happens with different HTTP methods. Fixes octokit/request.js#506.
timrogers
force-pushed
the
timrogers/content-length
branch
from
3e0dae5 to
25f5297
Compare
|
Following conversation with @gr2m, I have stripped back this PR to do the bare minimum. I'm going to run a manual test against the oldest supported GitHub Enterprise Server version, v3.2, to check it works with that. |
|
🎉 This PR is included in version 7.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Currently,
DELETErequests without a body don't work on the latest Node.js version, v18, because we try to setcontent-length: 0which is not allowed by the nativefetchimplementation which is included in Node. It throws an error with theUND_ERR_REQ_CONTENT_LENGTH_MISMATCHcode.This stops manually setting that
content-length: 0header forDELETErequests.I've tested this against the GitHub API with Node.js v18and v17, and both work as expected, with the GitHub API also accepting the request.
As part of this PR, I've also added some new tests covering what happens with different HTTP methods.
Fixes octokit/request.js#506.