feat: v8

[nickfloyd]

BREAKING CHANGE: Replace support for Node.js http(s) Agents with documentation on using fetch dispatchers instead

RE: octokit/octokit.js#2484

Node's native fetch method does not support using a Node http(s) agent, because it's not built on top of Node's http module (it's built on top of the net module instead).

---

Behavior

Before the change?

• Users of the @octokit/request module could pass in request options to fetch and request would map those

After the change?

• Users should now implement a custom fetch function with a dispatcher preset

---

Additional info

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes (Please add the Type: Breaking change label)
• No

---

[wolfy1339]

This removes all options, which would be undesirable.

[nickfloyd]

@wolfy1339 Thanks for the introspection here.

@gr2m and I had a conversation about this change where three different scenarios presented themselves as to how to approach this (I'll paraphrase, and hopefully, @gr2m will jump in so that we can have a complete conversation about it).

Here are the 3 options:

1. Remove requestOptions.request as any (this) and fix the breaking references
2. Remove any options that we implement in our fetch wrapper that are web standards only - removing all of the ones based on node-fetch - specifically around the standards defined here and removing any node-fetch here specific options.
3. Remove all options passed and mapped in our fetch wrapper. While this is the highest degree of breaking change, this does exactly what we need to decouple things. i.e. removing the Object.assign in the fetch-wrapper.ts - here - like I've done in this PR.

What do you think about the above? If you get a chance, please share why you think option three would be less desirable.

I could definitely be missing something obvious as well, so your thoughts and perspective here would be appreciated!

[wolfy1339]

I believe that transparently passing options to the fetch function, would be ideal.

That way we don't do any specific handling on our side, and users don't have to use a custom fetch function when they need to pass options.

We can then remove documentation on the options, and direct users to the documentation for their implementation of that fetch function, whether it be node-fetch (or another similar library) or the Fetch API.

That would offer some lesser level of breaking change for users

[nickfloyd]

Hey @wolfy1339, thanks for the thoughts/feedback - we're you thinking the implementation would look something like this?

[wolfy1339]

Changes need to be made to @octokit/types first before merging this, to remove the node-fetch specific options:

• agent
• timeout (can be replaced with docs on how to use the signal option with AbortSignal, see the v3 upgrade guide of node-fetch)

---

Currently, this won't work as-is.
The requestOptions contains all options for the request which is non-standard, and custom to our API.

Here is a couple points I'm seeing that will cause conflict with this PR:

• This would break all requests that need to use different HTTP methods as we don't pass this information along.
• For POST, or PUT requests, one needs to be able to pass a body to the HTTP request.
• Since we aren't passing along headers, which are needed for the requestOptions.mediaType options, those requests would default to use json instead of the intended mediatype format

[wolfy1339]

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)

[kfcampbell]

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.

[wolfy1339]

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

[wolfy1339]

Even with the most recent changes, this won't work.

Continuing on with the thoughts on my previous comment,

For option 3, (or is it option 1) only allow setting headers, method, url, body, redirect options, and the duplex option (as it is needed when there is a body) since those options are needed for downstream modules.

To pass through options, we would still require the Object.assign() there isn't a way around that, it would satisfy option 2

[gr2m]

how about we add options.request.fetchOptions, and fetchOptions can contain all fetch options that the native fetch supports? That way we don't need merge all of options.request into the fetch request call. And it would make it all explicit.

I was initially preferring to just support options.request.fetch, but I can now imagine that there will be valid use cases where we can't set fetch, but still want to pass fetch options such as a signal in order to abort a request. For example in the context of plugins, we cannot override the fetch used by the octokit instance, but we still might want to pass a signal on a per-request basis.

[wolfy1339]

how about we add options.request.fetchOptions, and fetchOptions can contain all fetch options that the native fetch supports? That way we don't need merge all of options.request into the fetch request call. And it would make it all explicit.

👍 This is a good idea

[nickfloyd]

how about we add options.request.fetchOptions, and fetchOptions can contain all fetch options that the native fetch supports? That way we don't need merge all of options.request into the fetch request call. And it would make it all explicit.

This seems like a reasonable approach. Concretely you are suggesting the following (please correct me if I am wrong on anything so that I know we are all on the same page):

1. Create a new option named options.request.fetchOptions which is the container for the explicit options from native fetch (according to this) which are:

• options.method ✅
• options.headers ✅
• options.body ✅
• options.mode ✅
• options.credentials❓ (NOTE: supporting this in fetchOptions does not seem right given how we handle auth)
• options.credentials.omit❓(should we support this?)
• options.credentials.same-origin❓(should we support this?)
• options.credentials.include❓(should we support this?)
• options.cache ✅
• options.redirect ✅
• options.referrer❓(should we support this?)
• options.referrerPolicy
• options.integrity❓(should we support this?)
• options.keepalive ✅
• options.signal ✅
• options.priority❓(should we support this?)

Some of the options above seem superfluous/unnecessary for the type of support we're trying to give, the way API requests are made to our API - things like: options.credentials.*, options.referrer, options.integrity, and options.priority

2. Keep

• route
• options.baseUrl
• options.url (mapped as Fetch resource)
• options.mediaType.format
• options.request.log
• options.mediaType.previews (since this library also allows interactions with GraphQL we have to keep this one in)
• options.request.hook

4. Remove the following options in the docs as well as how they are referenced/used in the source in favor of the above implementation:

• options.headers
• options.method
• options.data
• options.request.agent
• options.request.signal

5. Update any related or currently missing types that support these parameters in @octokit/types here and reference in the beta here.

Let me know if all of this sounds right or what needs to be adjusted.

[kfcampbell]

I think @nickfloyd's take is reasonable; we shouldn't support other ways of passing in authentication. I'm ready/willing to carry out this work if we can get consensus on the approach.

[gr2m]

7. Remove the following options in the docs as well as how they are referenced/used in the source in favor of the above implementation:

• options.headers
• options.method
• options.data
• options.request.agent
• options.request.signal

I think removing the 1st three will result in a major pain, I would keep these as first class citizen options. Or at least be very cautious about removing them, do a proper deprecation to give people time to move away from it.

But even if painful, I think it's the right call at this point. Make octokit less magic, rely more on web standards fetch, it's a win-win in the long term.

1. Create a new option named options.request.fetchOptions which is the container for the explicit options from native fetch (according to this) which are:

• options.method ✅
• options.headers ✅
• options.body ✅
• options.mode ✅
• options.credentials❓ (NOTE: supporting this in fetchOptions does not seem right given how we handle auth)
• options.credentials.omit❓(should we support this?)
• options.credentials.same-origin❓(should we support this?)
• options.credentials.include❓(should we support this?)
• options.cache ✅
• options.redirect ✅
• options.referrer❓(should we support this?)
• options.referrerPolicy
• options.integrity❓(should we support this?)
• options.keepalive ✅
• options.signal ✅
• options.priority❓(should we support this?)

I would start out with only supporting options explicitly that we know are useful. Adding more fetch options in future is simple, but removing them if we run into any complications will be a pain.

I'm not actually sure what they all do and how they are supported. I'd definitely start out with method, headers, body, and signal. I'm not sure about the rest, and would probably vote no but instead open an issue to discuss them before adding them

[gr2m]

• options.credentials❓ (NOTE: supporting this in fetchOptions does not seem right given how we handle auth)

I agree. Authentication is a first-class citizen concern for us. I was wondering in the past if we should support the same authentication options as we support for the Octokit constructor in .request(), but that's out of scope of this discussion.

[kfcampbell]

Here's a follow-up based on Nick's plan above:

1. Create a new option named options.request.fetchOptions which is the container for the following subset of the explicit options from native fetch (according to this):

• options.method ✅
• options.headers ✅
• options.body ✅
• options.signal ✅

2. Keep

• route
• options.baseUrl
• options.url (mapped as Fetch resource)
• options.mediaType.format
• options.request.log
• options.mediaType.previews (since this library also allows interactions with GraphQL we have to keep this one in)
• options.request.hook

3. Remove the following options in the docs as well as how they are referenced/used in the source in favor of the above implementation:

• options.request.agent
• options.request.signal

4. Implement the following options with an alias, log a deprecation warning, and either note deprecation in docs or remove docs entirely:

• options.headers
• options.method
• options.data

4. Update any related or currently missing types that support these parameters in @octokit/types here and reference in the beta here.

Thoughts?

[wolfy1339]

I agree with the plan. It seems well thought out

[kfcampbell]

Step one in the plan presented here exists on this PR in its current state. Should the other steps be part of this PR as well or split up (perhaps using other PRs into this branch)?

[wolfy1339]

I think that for now, we can leave this PR as-is in order to unblock downstream updates ASAP.

Further follow-ups can be made for the remaining points

[kfcampbell]

Sounds good! I've marked the PR as ready for review. Since @nickfloyd and I were the committers, I'll wait for approval/merge from you or @gr2m before proceeding with rolling out updates across other repos.

[wolfy1339]

octokit/types.ts#561 needs to be merged as a prerequisite to this PR

The version will need to be updated in this PR as well

[kfcampbell]

👍 I've marked that PR as ready for review and I'll update the version here once it's released.

[wolfy1339]

That PR has been merged

[kfcampbell]

Thanks! Package has been updated here. After this, according to the plan, I believe the idea is to update core.js first, then octokit.js, app.js, and rest.js. Is that correct?

[wolfy1339]

We need to update the dependencies of @octokit/core first, and so on.

Then the dependencies of @octokit/rest
Then dependencies of @octokit/app

Octokit.JS is the final one to get updated, as it depends on all the other modules

[github-actions]

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀