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.

Sign up for GitHub

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

Jump to bottom

feat: v8 #599

Merged
merged 15 commits into from
Jul 7, 2023
Merged

feat: v8 #599

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2785a42
Breakinging change: stop passing objects to fetch in the fetch-wrapper
nickfloyd Jun 29, 2023
8a4f452
passes requestOptions through and making it optional
nickfloyd Jun 29, 2023
74d9b7c
updates types - actual octokit/types version will be v11.x
nickfloyd Jun 30, 2023
0592ad9
removes the default on the options parameter some of the options allo…
nickfloyd Jun 30, 2023
4ed1770
puts options.method docs back in
nickfloyd Jul 5, 2023
45f086b
Previews are only for GraphQL
kfcampbell Jul 6, 2023
035d865
Update README.md
kfcampbell Jul 6, 2023
35a84c6
Explicitly pass signal but no other request options
kfcampbell Jul 7, 2023
f174105
Add method, body, headers
kfcampbell Jul 7, 2023
f348708
Merge branch 'main' into beta. Use Object.assign to pass through data
kfcampbell Jul 7, 2023
1cfdf79
npm run lint:fix
kfcampbell Jul 7, 2023
b4f0bcd
Do not use Object.assign
kfcampbell Jul 7, 2023
4a80c29
npm run lint:fix
kfcampbell Jul 7, 2023
54c25e6
Remove skipped test
kfcampbell Jul 7, 2023
98259f7
Bump types version to v11
kfcampbell Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 1 addition & 23 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,17 +265,6 @@ const { data: app } = await requestWithAuth(
Name of previews, such as `mercy`, `symmetra`, or `scarlet-witch`. See <a href="https://developer.github.com/v3/previews/">API Previews</a>.
kfcampbell marked this conversation as resolved.
Show resolved Hide resolved
</td>
</tr>
<tr>
<th align=left>
<code>options.method</code>
</th>
<td>
String
</td>
<td>
Any supported <a href="https://developer.github.com/v3/#http-verbs">http verb</a>, case insensitive. <em>Defaults to <code>Get</code></em>.
</td>
</tr>
gr2m marked this conversation as resolved.
Show resolved Hide resolved
<tr>
<th align=left>
<code>options.url</code>
Expand All @@ -299,18 +288,7 @@ const { data: app } = await requestWithAuth(
Set request body directly instead of setting it to JSON based on additional parameters. See <a href="#data-parameter">"The `data` parameter"</a> below.
</td>
</tr>
<tr>
<th align=left>
<a name="options-request-agent"></a>
<code>options.request.agent</code>
</th>
<td>
<a href="https://nodejs.org/api/http.html#http_class_http_agent">http(s).Agent</a> instance
</td>
<td>
Node only. Useful for custom proxy, certificate, or dns lookup.
</td>
</tr>

<tr>
<th align=left>
<code>options.request.fetch</code>
Expand Down
18 changes: 1 addition & 17 deletions src/fetch-wrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,7 @@ export default function fetchWrapper(
);
}

return fetch(
requestOptions.url,
Object.assign(
{
method: requestOptions.method,
body: requestOptions.body,
headers: requestOptions.headers as HeadersInit,
redirect: requestOptions.redirect,
// duplex must be set if request.body is ReadableStream or Async Iterables.
// See https://fetch.spec.whatwg.org/#dom-requestinit-duplex.
...(requestOptions.body && { duplex: "half" }),
},
// `requestOptions.request.agent` type is incompatible
// see https://github.com/octokit/types.ts/pull/264
requestOptions.request as any
)
)
return fetch(requestOptions.url, requestOptions as any || {})
.then(async (response) => {
url = response.url;
status = response.status;
Expand Down
3 changes: 2 additions & 1 deletion test/request.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,8 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w==
);
});

it("options.request.signal is passed as option to fetch", function () {
//TODO: figure out the expected behavior
it.skip("options.request.signal is passed as option to fetch", function () {
Copy link
Member

Choose a reason for hiding this comment

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

Linking docs for the signal option for clearer context: https://developer.mozilla.org/en-US/docs/Web/API/fetch#signal

This test seems like a sanity check to make sure the option is passed along by passing a value that isn't the expected type and checking if it throws an error

Example:

> fetch("https://example.com",{signal:"bar"})
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 294,
  [Symbol(trigger_async_id_symbol)]: 5
}
> Uncaught:
TypeError: Failed to construct 'Request': member signal is not of type AbortSignal.
    at Object.fetch (node:internal/deps/undici/undici:11457:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Copy link
Member

Choose a reason for hiding this comment

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

In that case, with these breaking changes, should this test be removed? If we're expecting the caller to bring their own fetch, perhaps we shouldn't test on the internals of those options.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that this is the only test that checks if the options were properly passed along.

I think it should be fine to remove it

return request("/", {
request: {
// We pass a value that is not an `AbortSignal`, and expect `fetch` to
Expand Down