Pass node-fetch as options.request.fetch to the Octokit constructor

[nickfloyd]

After speaking with @gr2m we decided to pass in node-fetch (as an option) to the Octokit constructor now that request.js has changed.

This should ensure that the action will work as is without requiring a node bump and or changes on the consumer side (avoiding breaking changes)

Relates to #221

---

Behavior

Before the change?

• Prior to this change a breaking change was going to be introduced as part of this effort because of the change around support for Node's HTTP agent.

After the change?

• There should no longer be a need for a breaking change to be introduced here and theoretically, this should still be able to "support" node v16. The end goal would be to remove this once Actions moves to Node v18 as their runtime.

[wolfy1339]

Since the other Octokit modules that power this one dropped support for Node 16, it is going to be harder to keep support

[nickfloyd]

Since the other Octokit modules that power this one dropped support for Node 16, it is going to be harder to keep support

Agreed, and I don't like having a special case; can you think of another way to handle this?

Currently, these are all kinda blocking each other:

• This fix(deps): update octokit monorepo (major) action.js#513 needs to have it's proxy approach fixed here and tests fixed
• This https://github.com/octokit/request-action needs the changes as a dep to be updated from above but is blocked by the changes needed.
• This fix(deps): update dependency @octokit/action to v6 #221 needs the changes as a dep to be updated from above, but that causes a problem because GH Actions does not support Node v18 - which is the reason for this PR.

I'm trying to find a clean path to move forward, but I keep walking into the agent support / node v16 walls. I'd love to hear thoughts or clarity from all that have been involved so far (@gr2m, @wolfy1339, @kfcampbell, @oscard0m) - I might be overthinking this or simply in need of a better perspective. I am sure we can come up with a decent path to get these last 3 libraries landed.

Let me know y'alls thoughts.

[wolfy1339]

Undici supports Node 16, no problem.
We could use that so that every platform has a similar experience

They implement an AgentProxy() that we can use to replace https-proxy-agent.
It seems a pretty easy replacement

In @octokit/action these are the changes I would propose:

diff --git a/src/index.ts b/src/index.ts
index 0998398..f293763 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -6,7 +6,7 @@ export type { RestEndpointMethodTypes } from "@octokit/plugin-rest-endpoint-meth
 
 import { VERSION } from "./version";
 import type { OctokitOptions } from "@octokit/core/dist-types/types";
-import { HttpsProxyAgent } from "https-proxy-agent";
+import { fetch as undiciFetch, ProxyAgent } from "undici";
 
 const DEFAULTS = {
   authStrategy: createActionAuth,
@@ -17,16 +17,22 @@ const DEFAULTS = {
 function getProxyAgent() {
   const httpProxy = process.env["HTTP_PROXY"] || process.env["http_proxy"];
   if (httpProxy) {
-    return new HttpsProxyAgent(httpProxy);
+    return new ProxyAgent(httpProxy);
   }
 
   const httpsProxy = process.env["HTTPS_PROXY"] || process.env["https_proxy"];
   if (httpsProxy) {
-    return new HttpsProxyAgent(httpsProxy);
+    return new ProxyAgent(httpsProxy);
   }
 
   return undefined;
 }
+const customFetch = async function(url: string, opts: any) {
+  return await undiciFetch(url, {
+    dispatcher: getProxyAgent(),
+    ...opts
+  });
+};
 
 export const Octokit = Core.plugin(
   paginateRest,
@@ -36,7 +42,7 @@ export const Octokit = Core.plugin(
     ...DEFAULTS,
     ...options,
     request: {
-      agent: getProxyAgent(),
+      fetch: customFetch,
       ...options.request,
     },
   };

[gr2m]

I like @wolfy1339's approach 👍🏼

[nickfloyd]

Implemented here: octokit/action.js#513

[kfcampbell]

Closing this as the fetch changes were handled in #221.