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.

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

Use fetch instead of axios #314

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Use fetch instead of axios #314

wants to merge 39 commits into from

Conversation

glynnbird
Copy link
Contributor

@glynnbird glynnbird commented Dec 16, 2022

Overview

Replaces axios HTTP library with fetch, powered-by undici. The upshot of this is that we reduce the number of dependencies to zero.

Comments and advice welcome.

fetch

Some history: originally Nano was built on top of the request library which was later deprecated. At this point I reworked it to use axios instead. This PR eliminates axios and other axios-related dependencies and instead uses the new kid on the block: the fetch API.

The fetch feature has found widespread adoption in web browsers as a means of handling outbound HTTP requests. It has found its way into Node.js as a global function and is marked as an experimental feature in Node 18/19 and is mainstream in Node 20 and beyond.

Node.js's fetch capability is powered by the undici package which is bundled with Node.js and in turn uses Node's low-level network libraries instead of being based on the higher-level http/https built-in modules. It purports to be significantly faster (according to its own benchmarks) than traffic routed through http/https modules, as is the case with other HTTP libraries like axios & request.

Automated testing

Replacing axios with fetch means also ditching the nock library for mocking HTTP requests and responses, because nock works by intercepting requests originating from the http/https layer, which is bypassed by undici. Fortunately, undici provides its own Mocking tooling.

The outcome

This branch's needs no runtime dependencies. Current dependencies:

  "dependencies": {
    "http-cookie-agent": "^4.0.2",
    "@types/tough-cookie": "^4.0.2",
    "axios": "^1.1.3",
    "qs": "^6.11.0",
    "tough-cookie": "^4.1.2",
    "node-abort-controller": "^3.0.1"
  },
  "devDependencies": {
    "@types/node": "^18.11.9",
    "jest": "^29.2.2",
    "nock": "^13.2.9",
    "standard": "^17.0.0",
    "typescript": "^4.8.4"
  }

Post-PR dependencies:

  "dependencies": {
  },
  "devDependencies": {
    "undici": "^6.2.1",
    "@types/node": "^22.3.0",
    "typescript": "^5.5.4"
  }

Backwards compatibility

None of Nano's API has changed except when a user is supplying non-default connection handling parameters. Gone is requestDefaults which dates back to the "request" days and instead an optional agentOptions can be provided which is documented in the README and in TypeScript.

const agentOptions = {
  bodyTimeout: 30000,
  headersTimeout: 30000,
  keepAliveMaxTimeout: 600000,
  keepAliveTimeout: 30000,
  keepAliveTimeoutThreshold: 1000,
  maxHeaderSize: 16384,
  maxResponseSize: -1,
  pipelining: 6,
  connect: { 
    timeout: 10000
  },
  strictContentLength: true,
  connections: null,
  maxRedirections: 0
}
const undici = require('undici')
const undiciOptions = new undici.Agent(agentOptions)
const nano = Nano({ url: 'http://127.0.0.1:5984', undiciOptions })

Node versioning

As Node 16 is EOL, we can release v11 of Nano and make it the Node 18+ version. The v10 series of Nano would still be supported for the time being for older versions of node.

Testing recommendations

The test suite has been rewritten to use the built-in Node.js test runner (one fewer dependency!) and uses the undici.MockAgent to simulate responses for each of Nano's API calls, just as Nock did previously.

Run with:

npm run test

GitHub issue number

Fixes #307

Related Pull Requests

n/a

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@glynnbird
Copy link
Contributor Author

glynnbird commented Dec 21, 2022

My simple benchmarking of small HTTP requests measures that Nano "11" would be 0 to 10% faster than Nano 10.1.

Testing method: create simple Node Express app to deliver canned responses to nano requests. Time Nano 10.1 & Nano 11 in doing different nano calls over many cycles and calculate the average response time (ms).

Nano 10.1 Nano "11" % speed up
/_all_dbs 0.59 0.5309 10.0
/db/id 0.573 0.543 5.2
/db/_all_docs?limit=100&include_docs=true 0.793 0.779 1.8
/db/_all_docs?limit=10000&include_docs=true 23.425 23.388 0.2

^ my reading of these findings is that the smaller the response payload, the more the efficiency gains of using undici for outbound requests shows up. For larger response bodies, the benefit is swallowed up by the physics of transferring packets over the network.

@glynnbird glynnbird marked this pull request as draft January 26, 2023 14:30
@glynnbird glynnbird changed the title Refactor to use fetch instead of axios Use fetch instead of axios May 3, 2023
@2colours
Copy link

@glynnbird Hi, any update or idea about how to progress this? It's not just one less dependency but axios itself has questionable code quality - along with the workarounds that currently make this package work with axios.

Copy link
Member

@janl janl left a comment

Choose a reason for hiding this comment

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

Very nice work, let’s get this sorted quickly (heh), just left a few questions and notes.

Some of the questions answer themselves in later commits, I left them in as a log of what was reviewed.

NOTICE Outdated Show resolved Hide resolved
lib/nano.js Outdated Show resolved Hide resolved
try {
retval = await response.json()
} catch {
// do nothing
Copy link
Member

Choose a reason for hiding this comment

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

would this hide errors when reading the response body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would. I'll fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realised why this is there. When doing HEAD /db/id there is no body so response.json throws an error.

Copy link
Member

Choose a reason for hiding this comment

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

ah, a comment would be nice then :)

Copy link
Member

Choose a reason for hiding this comment

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

and re-throw if method != head?

try {
body = await response.json()
} catch (e) {
body = ''
Copy link
Member

Choose a reason for hiding this comment

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

could do with a little more explanation, why are we doing this? And if .json() fails, is there a way to get .text() instead (not sure because consumed streams and all)

Copy link
Member

Choose a reason for hiding this comment

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

or rather, should we make .json() vs .text() dependent on the response Content-Type?

Copy link
Contributor Author

@glynnbird glynnbird Nov 4, 2024

Choose a reason for hiding this comment

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

If the content type is json, we use response.json to retrieve the body. (we use response.text or response.arrayBuffer for other content types). The try/catch is there because response.json throws an error if the body is empty - which occurs for HEAD /db/id for example.

if (typeof body === 'string') {
body = { message: body }
}

if (!body.message && (body.reason || body.error)) {
if (body && !body.message && (body.reason || body.error)) {
Copy link
Member

Choose a reason for hiding this comment

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

is this because of the body = '' in line 200?

Copy link
Member

Choose a reason for hiding this comment

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

if yes, we should make that relationship more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh. I can't remember why the code is like that. I think it goes back to Nano when it was built on request, then built on axios and now built on fetch

Copy link
Member

Choose a reason for hiding this comment

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

I just mean the addition of the body shortcut in front

test/database.changes.test.js Outdated Show resolved Hide resolved
lib/nano.js Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@janl
Copy link
Member

janl commented Nov 3, 2024

also: should we drop the callback variant?

@janl
Copy link
Member

janl commented Nov 3, 2024

and: do we need a 10_to_11 migration guide?

glynnbird and others added 5 commits November 4, 2024 10:15
Co-authored-by: Jan Lehnardt <jan@apache.org>
…no longer look for exact strings being returned in the assertion failure as that is flaky
@glynnbird
Copy link
Contributor Author

glynnbird commented Nov 4, 2024

@janl

also: should we drop the callback variant?

It's pretty easy to stop the code handling callbacks, then it's just removing some tests, changing the README and migration guide. It would be a bigger breaking change ofc. what do you think?

@janl
Copy link
Member

janl commented Nov 6, 2024

It would be a bigger breaking change ofc. what do you think?

we don’t have to drop callbacks in this PR, but when we release main with this PR, we should bump the major version number and when we do that, we can also drop the callback api

@glynnbird
Copy link
Contributor Author

I went ahead and removed callbacks from the code, the Typescript definitions, the tests, the migration guide and the README. The work is in a branch here https://github.com/apache/couchdb-nano/tree/remove-callbacks (branched from the fetch branch).

We can either

  1. Release the fetch branch as Nano 11 and then have a further release later on to remove callbacks
  2. or release fetch and remove callbacks at the same time in Nano 11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fetch instead of axios
3 participants