Capture error message, otherwise capture cause of HTTP failure

[digitalextremist]

One approach to #356; refactor likely needed ( along with #355 ) to wring out design errors.

Overview

Intended to resolve #356

That was originally seen as fixing a syntax error, but the syntax error existed because of a theory error.

By that theory error, when an Error was thrown on a request, the message is being lost.

Reasoning

This has already met !response.status which means it is more than likely error and not response being called response due to the theory error, since the design does not have a separate responseHandler for errors.

Because of that, I check response.message first, then response.cause?.toString() ( includes fix requested by #356 )

Testing recommendations

• Use with no server online ( per Server crashes because response.cause doesn't exist #356 (comment) case )

---

Since this is a small change of one line ... but might trigger a refactor since there is a theory error still:

• No documentation change required
• Putting this out there for discussion first
• Have run this in my environment
• May need a test added, triggering error
• Refactor likely needed which might invalidate

[digitalextremist]

Not sure if this codebase favors ECMAScript 2020 but I assumed so because node >=20 is required and there is talk of modernizing. Dipping my toe in since I love nano and CouchDB so feel free to nudge toward a standard practice.

[glynnbird]

This fix seems like a reasonable simple iteration to me.

I tried it with

import Nano from './lib/nano.js'
const nano = Nano('http://localhost:9999')

async function main() {
  await nano.db.list()
}
main()

Works fine (in that you get an error with a sensible error message) in Node, but a reproducer of your problem when using bun. It seems to come down do different formats of responses in Node & Bun.

Your ?? trick works just fine and I'm happy to merge it in.

[glynnbird]

I tried to run the tests in bun with bun test test/*.test.js but it doesn't seen to like Undici's MockAgent, which is used to intercept the library's HTTP calls during testing.

[digitalextremist]

Thanks @glynnbird; I also reproduced the fix with Bun 1.2.22 now:

Showed the before and after there.

Proud to bring a commit to this awesome repository once merged!

[glynnbird]

if you remove "WIP" and request a review from me I can get it moving.

[glynnbird]

Actually if we go with:

response.message || response.cause.toString()

then Node errors always get "fetch failed" which is not a great error message, not as good as Error: connect ECONNREFUSED 127.0.0.1:9999

what about

response.cause ? response.cause.toString() : response.message

instead? I reckon most users are Node users anyway?

[digitalextremist]

Removed WIP and here is the follow-up reasoning from someone who looks both ways when crossing one-way streets:

In the hypothetical, there is the possibility that response.message does not exist; versus in the change as proposed, we know for sure response.message is there and not null ever.

---

In this approach:

response.statusText = response.message ?? response.cause?.toString()

I have tried to move away from || over to ?? if possible, in general, because || is 'falsey' versus ?? which is 'nullish' which is what we are looking for in this case. Same with ? there.

Looking ahead 10 years: This is almost to the point of being able to use ??= but not quite; likely after a refactor... until then and after, the newer operators are still smaller in token-size and download size after minification.

---

The underlying question regarding node vs. bun error messages is likely more the decision versus syntax, and tricky; I feel like that too is in the 10-year view because there is also deno and are other engines, and they will come and go.

What is best for us? I tend to think fast code without any possibility of an error due to ghosts and aliens is best for us.

From there, I defer! I am not the one who got this far, I only hope to go forward with nano as far as this has come, at least. I do question, pretty much daily: while I will be able to stick with CouchDB and this package? I find myself truly hoping so!

[digitalextremist]

Added a comment since it seems appropriate in this case: abstractive@1537ad8

[glynnbird]

My point is that if we go with your PR as is (where priority is given to the response.message field) the Node.js user gets a worse experience.

	response.cause	response.message
Node	Error: connect ECONNREFUSED 127.0.0.1:9999	fetch failed
Bun	undefined	Unable to connect. Is the computer able to access the url?

Basically we response.cause is the best option for Node and response.message is the best option for Bun.

[digitalextremist]

I agree! But... some forest:

The entire section is kinda moot and has code-smell, because it rests on response.statusText then never uses the complexity there, needless operations happen, items happen out of order by priority, etc... since it just returns undefined anyway and we are just looking building the error message by then.

As a bit more of an aggressive/refactory PR then, I optimized the piece going to run either way ( responseHeaders being populated by Object.assign rather than a faster and more readable spread ) ... then go for the gold on specificity, since we are still suppressing ( for the programmer ) what is going on there and by this point, performance became a lost cause:

    const responseHeaders = {
      uri: scrubURL(req.url),
      statusCode,
      ...(response.headers ?? {})
    };
  
    if (!response.status) {
      log({ err: 'socket', body, headers: responseHeaders })
      if (reject) {
        // since #relax might have sent Error rather than Response:
        const statusText = response.cause?.toString() ?? response.message
        reject(new Error(`error happened in your connection. Reason: ${statusText}`))
      }
      return
    }

What do you think? Beyond a happy-medium and get some refactoring started?

[glynnbird]

This is better. There's a bunch of stuff in the code that has festered from the request --> axios --> fetch generations, so there's definitely a nicer-looking solution out there (in the future).

I'm more worried about not giving node users a worse error message and your latest code snippet fixes that.

[digitalextremist]

I feel the tension! I like to start to turn the overall insurmountable-feeling table while dealing with the immediate. I hope to help, piece by piece, if I can. I admire the care for how the various engines will behave. Hopefully this PR works out.

That snippet is the new state of the branch; let me know if there are any other concerns that come to mind @glynnbird.

[glynnbird]

Thanks for the tidy up while still giving the Node user the "cause" and the Bun user the "message" 👍