You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "digitalextremist (via GitHub)" <gi...@apache.org> on 2025/09/14 05:10:37 UTC
[PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
digitalextremist opened a new pull request, #358:
URL: https://github.com/apache/couchdb-nano/pull/358
One approach to apache/couchdb-nano#356
## 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 the fitch requested by #356 )
## Testing recommendations
- [ ] Use with no server online ( per https://github.com/apache/couchdb-nano/issues/356#issuecomment-3287835196 case )
---
Since this is a small change of one line ... but might trigger a refactor since there is a theory error still:
- [ ] I am putting this out there for discussion first
- [ ] I have not run this in my environment yet
- [ ] Still needs a test added
- [ ] Refactor more likely needed which will invalidate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291234913
if you remove "WIP" and request a review from me I can get it moving.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "digitalextremist (via GitHub)" <gi...@apache.org>.
digitalextremist commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291891449
Added a comment since it seems appropriate in this case: https://github.com/abstractive/couchdb-nano/commit/1537ad88bcf7b7f00ec90bba548bb81bd56da0ff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "digitalextremist (via GitHub)" <gi...@apache.org>.
digitalextremist commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291222078
Thanks @glynnbird; I also reproduced the fix with `Bun 1.2.22` now:
<img width="908" height="620" alt="image" src="https://github.com/user-attachments/assets/431c3a66-a9a5-4461-a206-7a9d1b9c1b5c" />
Proud to bring a commit to this awesome repository once merged!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3292010913
My point is that if we got 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291162612
This fix seems like a reasonable simple iteration to me.
I tried it with
```js
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "digitalextremist (via GitHub)" <gi...@apache.org>.
digitalextremist commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3289360368
Not sure if this codebase allows `ECMAScript 2020` but I assumed so because `node >=20` is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "digitalextremist (via GitHub)" <gi...@apache.org>.
digitalextremist commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3292801075
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "digitalextremist (via GitHub)" <gi...@apache.org>.
digitalextremist commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291650551
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`
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, the newer operations are also smaller in token-size and download size after minification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird merged PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291283112
Actually if we got 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
```js
response.cause ? response.cause.toString() : response.message
```
instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "digitalextremist (via GitHub)" <gi...@apache.org>.
digitalextremist commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3292230978
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3292381602
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 does just that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ WIP ] Capture error message, otherwise capture cause of HTTP failure [couchdb-nano]
Posted by "glynnbird (via GitHub)" <gi...@apache.org>.
glynnbird commented on PR #358:
URL: https://github.com/apache/couchdb-nano/pull/358#issuecomment-3291203153
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org