Replace axios with fetch API

[snoack]

Replace axios with fetch API

Backwards-incompatible changes:

• Removed support for passing through an agent object on Node.js; fetch does not support this. reviewable-server was using this mechanism to enable keep-alive via agentkeepalive. Node.js' fetch implementation does keep-alive by default but with somewhat different configuration than reviewable-server used to apply:
  • Idle/free socket timeout: Was 10s; now defaults to 4s, but it may be overridden by server Keep-Alive hints up to 10min.
  • Active socket timeout: Was 20s; fetch has a header timeout (5min), body timeout (5min), and connect timeout (10s), but no "working socket inactivity timeout".
  • TCP keepalive initial delay: Was 500ms; now is 60s.
  • Max sockets: We used to cap free/idle sockets at 64 while allowing unlimited active sockets. fetch also allows unlimited active connections but has no way of limiting idle connections.
  • Metrics: We used to log stats reported by agentkeepalive.getCurrentStatus(); subscribe to undici:* events with diagnostics_channel instead.
• Removed retry behavior for presumed CORS failures.
• Removed support for responseType='document'. This is not supported by the fetch API and is unused in Reviewable.
• The shape of errors thrown by fetch differs from those thrown by axios, most notably error.code and error.timeout are gone; use error.name === 'AbortError' to detect timeouts and error.networkFailure.

---

This change is 

[pkaminski]

@codex review

Triggered automatically because this PR was opened ready for review or was marked ready for review.

[pkaminski]

I see a PR for testing integration with reviewable-server but not one for reviewable-client. Definitely need that, in both the main process and diff_worker.

@pkaminski reviewed 6 files and all commit messages, and made 10 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on snoack).

---

a discussion (no related file):

Removed support for passing through an agent object on Node.js; fetch does not support this. reviewable-server was using this mechanism to enable keep-alive via agentkeepalive. Node.js' fetch implementation does keep-alive by default but with somewhat different configuration than reviewable-server used to apply:

• Idle/free socket timeout: Was 10s; now defaults to 4s, but it may be overridden by server Keep-Alive hints up to 10min.
• Active socket timeout: Was 20s; fetch has a header timeout (5min), body timeout (5min), and connect timeout (10s), but no "working socket inactivity timeout".
• TCP keepalive initial delay: Was 500ms; now is 60s.
• Max sockets: We used to cap free/idle sockets at 64 while allowing unlimited active sockets. fetch also allows unlimited active connections but has no way of limiting idle connections.
• Metrics: We used to log stats reported by agentkeepalive.getCurrentStatus(); fetch doesn't provide an equivalent API.

The different timings should be fine, especially as they were originally tuned a decade ago and I'm sure GitHub has changed things on their side since. I am concerned about the lack of any stats as that'll make it harder to debug issues if we see a surge in disconnects or other GH API issues. No way around it, I guess — at worst, we'll need to revert.

Does fetch have a limit on how many sockets it'll open to the same host? If so, this could be an issue server-side.

We were also forcing the agent off go ensure a fresh connection when retrying after a network or server error. It's not clear to me whether that's actually useful or necessary, and perhaps fetch will do it automatically anyway for network errors. Not sure how to test this, though.

---

a discussion (no related file):

Removed fine-grained network error handling, including logic that retried failed CORS requests without a preflight. In the fetch API, CORS failures are indistinguishable from other network failures.

This could be a problem. IIRC GitHub will automatically redirect REST requests made under a repo's old name but the redirect will fail if the request needing preflighting. Then again, I think essentially all our requests need preflighting nowadays since we send an Authorization header and dropped support for passing the token via query params. Should still test this scenario to see how GitHub/browsers handle it nowadays though.

---

a discussion (no related file):

Removed support for responseType='document'. This is not supported by the fetch API and is unused in Reviewable.

Agreed, we only care about blob and text.

---

a discussion (no related file):

The shape of errors thrown by fetch differs from those thrown by axios, most notably error.code and error.timeout are gone; use error.name === 'AbortError' to detect timeouts and error.networkFailure.

This should be fine; hopefully the extra error messages we were snooping for also get reported as networkFailure.

---

hubkit.js line 451 at r2 (raw file):

                  1;
                if (options.stats) options.stats.record(false, size);
                if (res.status === 200 && (res.headers.get('etag') || res.headers.get('cache-control')) &&

Nit: 100 cols.

---

hubkit.js line 696 at r2 (raw file):

    if (options.responseType === 'arraybuffer') return response.arrayBuffer();
    if (options.responseType === 'blob') return response.blob();
    return response.text();

Nit: could be a switch.

---

package.json line 3 at r2 (raw file):

{
  "name": "hubkit",
  "version": "6.1.0",

You should bump the major version. (Our new release workflow just uses whatever's in the manifest, it doesn't bump it.)

---

hubkit.js line 343 at r2 (raw file):

              }
              if (!res.data && rawData &&
                  /\bformat=json\b/.test(res.headers['x-github-media-type'])) {

I think this worked around a randomly occurring bug in GitHub's REST responses some 12 years ago — hopefully they fixed it since.

---

hubkit.js line 495 at r2 (raw file):

          // If we get an error response without a status, then it's not a real error coming back
          // from the server but some kind of synthetic response Axios concocted for us.  Treat it
          // as a generic network error.

Is a fetch response guaranteed to have a status?

[snoack]

No changes — other than bumping the hubkit version — are require in reviewable-client as far as I can see.

@snoack made 7 comments and resolved 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on pkaminski).

---

a discussion (no related file):

I am concerned about the lack of any stats

Actually, I just learned there are some stats (though not the exact same metrics) we could observe in reviewable-server, and it wouldn't even require any support code in hubkit. The fetch implemantion in Node.js is backed by unidici which provides some metrics via the diagnostics_channel API.

Does fetch have a limit on how many sockets it'll open to the same host?

It does not.

perhaps fetch will do it automatically anyway

It does. Node.js' fetch implementation does not reuse TCP connections after socket failure.

---

a discussion (no related file):

Previously, pkaminski (Piotr Kaminski) wrote…

Removed fine-grained network error handling, including logic that retried failed CORS requests without a preflight. In the fetch API, CORS failures are indistinguishable from other network failures.

This could be a problem. IIRC GitHub will automatically redirect REST requests made under a repo's old name but the redirect will fail if the request needing preflighting. Then again, I think essentially all our requests need preflighting nowadays since we send an Authorization header and dropped support for passing the token via query params. Should still test this scenario to see how GitHub/browsers handle it nowadays though.

On second thought, it's not like that CORS failures were observable with axios. The old logic merely assumed any network failure to be a CORS failure. We could restore the same logic with fetch. But frankly, it seems like an odd workaround to me, and I'm inclined to see how things will play out if we leave it out for now. What do you think?

---

hubkit.js line 451 at r2 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Nit: 100 cols.

Fixed.

---

hubkit.js line 495 at r2 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Is a fetch response guaranteed to have a status?

It is; if fetch didn't successfully read and parse the HTTP headers, there won't be a response object but the promise returned by fetch gets rejected instead.

---

hubkit.js line 696 at r2 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Nit: could be a switch.

Done.

---

package.json line 3 at r2 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

You should bump the major version. (Our new release workflow just uses whatever's in the manifest, it doesn't bump it.)

Done. (I thought that would done as part of the release process.)

[pkaminski]

Did you test it thought? The client pushes the library into more edge cases than the server.

@pkaminski reviewed 2 files and all commit messages, made 5 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on snoack).

---

a discussion (no related file):

Previously, snoack (Sebastian Noack) wrote…

On second thought, it's not like that CORS failures were observable with axios. The old logic merely assumed any network failure to be a CORS failure. We could restore the same logic with fetch. But frankly, it seems like an odd workaround to me, and I'm inclined to see how things will play out if we leave it out for now. What do you think?

I think the code was vestigial anyway, as we lost the ability to make "simple requests" some time ago. The only potential exception would be for anonymous users but I see no reason to make a special case for them.

---

a discussion (no related file):
OK, let's add what metrics we can on the server then.

It does. Node.js' fetch implementation does not reuse TCP connections after socket failure.

I think we also dumped the socket after a 500-series error as a precaution, but perhaps that's too conservative.

---

package.json line 3 at r2 (raw file):

Previously, snoack (Sebastian Noack) wrote…

Done. (I thought that would done as part of the release process.)

It used to be but I standardized on a "push to release" flow for all repos, which doesn't leave space for deciding what to bump.

---

hubkit.js line 185 at r3 (raw file):

          if (options.onError) value = options.onError(error);
          if (value === undefined) {
            if (error.networkFailure || [500, 502, 503, 504].includes(res?.status)) {

We don't use ?. on the client, though I'm not sure why — perhaps just because we have ~ available? I'd be inclined to keep it out of client-side or isomorphic libraries as a precaution, though, unless you can confirm that it's OK.

[snoack]

I did some testing using this version of hubkit in reviewable-client, though I forgot about the diff_worker and truss_worker. I suppose I will test some more.

@snoack made 3 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pkaminski).

---

hubkit.js line 185 at r3 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

We don't use ?. on the client, though I'm not sure why — perhaps just because we have ~ available? I'd be inclined to keep it out of client-side or isomorphic libraries as a precaution, though, unless you can confirm that it's OK.

Hubkit already uses ?..

---

package.json line 3 at r2 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

It used to be but I standardized on a "push to release" flow for all repos, which doesn't leave space for deciding what to bump.

I was thinking it would be part of the manual part of the release process. But I suppose if you do a release for ever individual change, you might as well bump the version number as part of that change.