janl commented on code in PR #314:
URL: https://github.com/apache/couchdb-nano/pull/314#discussion_r1826962287
##########
lib/nano.js:
##########
@@ -168,29 +163,48 @@ module.exports = exports = function dbScope (cfg) {
delete responseHeaders.server
delete responseHeaders['content-length']
- /* if (opts.dontParse) {
- parsed = body
- } else {
- try { parsed = JSON.parse(body) } catch (err) { parsed = body }
- } */
-
if (statusCode >= 200 && statusCode < 400) {
- log({ err: null, body, headers: responseHeaders })
- if (resolve) {
- resolve(body)
+ // collect response
+ const ct = response.headers.get('content-type')
+ let retval = ''
+ if (ct === 'application/json') {
+ try {
+ retval = await response.json()
+ } catch {
+ // do nothing
+ }
+ } else if (ct && (ct.startsWith('text/') ||
ct.startsWith('multipart/related'))) {
+ retval = await response.text()
+ } else {
+ const ab = await response.arrayBuffer()
+ retval = Buffer.from(ab)
}
- if (callback) {
- callback(null, body, responseHeaders)
+
+ // log
+ log({ err: null, retval, headers: responseHeaders })
+
+ // promisey
+ if (resolve) {
+ resolve(retval)
+ } else if (callback) {
+ // callbacky
+ callback(null, retval, Object.fromEntries(response.headers))
}
return
}
// cloudant stacktrace
+ try {
+ body = await response.json()
+ } catch (e) {
+ body = ''
Review Comment:
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)
##########
lib/nano.js:
##########
@@ -168,29 +163,48 @@ module.exports = exports = function dbScope (cfg) {
delete responseHeaders.server
delete responseHeaders['content-length']
- /* if (opts.dontParse) {
- parsed = body
- } else {
- try { parsed = JSON.parse(body) } catch (err) { parsed = body }
- } */
-
if (statusCode >= 200 && statusCode < 400) {
- log({ err: null, body, headers: responseHeaders })
- if (resolve) {
- resolve(body)
+ // collect response
+ const ct = response.headers.get('content-type')
+ let retval = ''
+ if (ct === 'application/json') {
+ try {
+ retval = await response.json()
+ } catch {
+ // do nothing
+ }
+ } else if (ct && (ct.startsWith('text/') ||
ct.startsWith('multipart/related'))) {
+ retval = await response.text()
+ } else {
+ const ab = await response.arrayBuffer()
+ retval = Buffer.from(ab)
}
- if (callback) {
- callback(null, body, responseHeaders)
+
+ // log
+ log({ err: null, retval, headers: responseHeaders })
+
+ // promisey
+ if (resolve) {
+ resolve(retval)
+ } else if (callback) {
+ // callbacky
+ callback(null, retval, Object.fromEntries(response.headers))
}
return
}
// cloudant stacktrace
+ try {
+ body = await response.json()
+ } catch (e) {
+ body = ''
+ }
+
if (typeof body === 'string') {
body = { message: body }
}
- if (!body.message && (body.reason || body.error)) {
+ if (body && !body.message && (body.reason || body.error)) {
Review Comment:
is this because of the `body = ''` in line 200?
##########
NOTICE:
##########
@@ -1,5 +1,5 @@
Apache CouchDB Nano
-Copyright [2016-2018] The Apache Software Foundation
+Copyright [2016-2013] The Apache Software Foundation
Review Comment:
```suggestion
Copyright [2016-2024] The Apache Software Foundation
```
##########
test/database.changes.test.js:
##########
@@ -0,0 +1,100 @@
+// Licensed under the Apache License, Version 2.0 (the 'License'); you may not
Review Comment:
there it is
##########
README.md:
##########
@@ -4,6 +4,8 @@
Offical [Apache CouchDB](https://couchdb.apache.org/) library for
[Node.js](https://nodejs.org/).
+> Note: Nano >=11.0.0 is for Node 18/20 and above. If you are using Node 16 or
older, you need Nano 10.1.2.
Review Comment:
I feel we should be a bit more clear that 11+ is a breaking changes release
because of the http lib update
##########
lib/nano.js:
##########
@@ -168,29 +163,48 @@ module.exports = exports = function dbScope (cfg) {
delete responseHeaders.server
delete responseHeaders['content-length']
- /* if (opts.dontParse) {
- parsed = body
- } else {
- try { parsed = JSON.parse(body) } catch (err) { parsed = body }
- } */
-
if (statusCode >= 200 && statusCode < 400) {
- log({ err: null, body, headers: responseHeaders })
- if (resolve) {
- resolve(body)
+ // collect response
+ const ct = response.headers.get('content-type')
Review Comment:
style note, but would prefer `contentType` over `ct` for the usual reasons
##########
test/attachment.insert.test.js:
##########
@@ -82,31 +85,45 @@ test('should detect missing parameters (callback) -
db.attachment.insert', () =>
test('should be able to insert document attachment as stream - PUT
/db/docname/attachment - db.attachment.insert', async () => {
// mocks
const response = { ok: true, id: 'docname', rev: '2-456' }
- const scope = nock(COUCH_URL, { reqheaders: { 'content-type': 'image/jpg' }
})
- .put('/db/docname/logo.jpg?rev=1-150', image2)
- .reply(200, response)
+ mockPool.intercept({
+ method: 'put',
+ path: '/db/docname/logo.jpg?rev=1-150',
+ body: (value) => {
Review Comment:
this seems to be doing something other than the old test? or is all you’re
testing that `body` can be a streamy function rather than actually sending a
binary?
##########
lib/cookiejar.js:
##########
@@ -0,0 +1,45 @@
+const tough = require('tough-cookie')
+const cookieJar = new tough.CookieJar()
+
+// this is a monkey-patch of toughcookie's cookiejar, as it doesn't handle
+// the refreshing of cookies from CouchDB properly
+// see https://github.com/salesforce/tough-cookie/issues/154
Review Comment:
looks like this is fixed now?
https://github.com/salesforce/tough-cookie/pull/345
##########
test/attachment.insert.test.js:
##########
@@ -82,31 +85,45 @@ test('should detect missing parameters (callback) -
db.attachment.insert', () =>
test('should be able to insert document attachment as stream - PUT
/db/docname/attachment - db.attachment.insert', async () => {
// mocks
const response = { ok: true, id: 'docname', rev: '2-456' }
- const scope = nock(COUCH_URL, { reqheaders: { 'content-type': 'image/jpg' }
})
- .put('/db/docname/logo.jpg?rev=1-150', image2)
- .reply(200, response)
+ mockPool.intercept({
+ method: 'put',
+ path: '/db/docname/logo.jpg?rev=1-150',
+ body: (value) => {
+ return true
+ },
+ headers: {
+ 'content-type': 'image/jpg'
+ }
+ }).reply(200, response, JSON_HEADERS)
// test PUT /db/docname/attachment
const rs = fs.createReadStream('./test/logo.jpg')
const db = nano.db.use('db')
const reply = await db.attachment.insert('docname', 'logo.jpg', rs,
'image/jpg', { rev: '1-150' })
- expect(reply).toStrictEqual(response)
- expect(scope.isDone()).toBe(true)
+ assert.deepEqual(reply, response)
+ mockAgent.assertNoPendingInterceptors()
})
test('should be able to insert document attachment as stream with circular
reference - PUT /db/docname/attachment - db.attachment.insert', async () => {
// mocks
const response = { ok: true, id: 'docname', rev: '2-456' }
- const scope = nock(COUCH_URL, { reqheaders: { 'content-type': 'image/jpg' }
})
- .put('/db/docname/logo2.jpg?rev=1-150', image2)
- .reply(200, response)
+ mockPool.intercept({
+ method: 'put',
+ path: '/db/docname/logo2.jpg?rev=1-150',
+ body: (value) => {
Review Comment:
same question about streamy `body` function
##########
lib/nano.js:
##########
@@ -168,29 +163,48 @@ module.exports = exports = function dbScope (cfg) {
delete responseHeaders.server
delete responseHeaders['content-length']
- /* if (opts.dontParse) {
- parsed = body
- } else {
- try { parsed = JSON.parse(body) } catch (err) { parsed = body }
- } */
-
if (statusCode >= 200 && statusCode < 400) {
- log({ err: null, body, headers: responseHeaders })
- if (resolve) {
- resolve(body)
+ // collect response
+ const ct = response.headers.get('content-type')
+ let retval = ''
+ if (ct === 'application/json') {
+ try {
+ retval = await response.json()
+ } catch {
+ // do nothing
Review Comment:
would this hide errors when reading the response body?
##########
lib/cookie.js:
##########
@@ -0,0 +1,130 @@
+const { URL } = require('url')
+
+// a simple cookie jar
+class CookieJar {
Review Comment:
tough-cookie seems to have been fixed, but we are going for no-des here?
https://github.com/salesforce/tough-cookie/issues/154
##########
lib/nano.js:
##########
@@ -238,7 +278,9 @@ module.exports = exports = function dbScope (cfg) {
log({ err: 'couch', body: message, headers: responseHeaders })
- stream.emit('error', error)
+ setTimeout(() => {
Review Comment:
why is the `setTimeout()` needed?
##########
test/database.changes.test.js:
##########
@@ -1,103 +0,0 @@
-// Licensed under the Apache License, Version 2.0 (the 'License'); you may not
Review Comment:
why does this get deleted?
##########
lib/nano.js:
##########
@@ -168,29 +163,48 @@ module.exports = exports = function dbScope (cfg) {
delete responseHeaders.server
delete responseHeaders['content-length']
- /* if (opts.dontParse) {
- parsed = body
- } else {
- try { parsed = JSON.parse(body) } catch (err) { parsed = body }
- } */
-
if (statusCode >= 200 && statusCode < 400) {
- log({ err: null, body, headers: responseHeaders })
- if (resolve) {
- resolve(body)
+ // collect response
+ const ct = response.headers.get('content-type')
+ let retval = ''
+ if (ct === 'application/json') {
+ try {
+ retval = await response.json()
+ } catch {
+ // do nothing
+ }
+ } else if (ct && (ct.startsWith('text/') ||
ct.startsWith('multipart/related'))) {
+ retval = await response.text()
+ } else {
+ const ab = await response.arrayBuffer()
+ retval = Buffer.from(ab)
}
- if (callback) {
- callback(null, body, responseHeaders)
+
+ // log
+ log({ err: null, retval, headers: responseHeaders })
+
+ // promisey
+ if (resolve) {
+ resolve(retval)
+ } else if (callback) {
+ // callbacky
+ callback(null, retval, Object.fromEntries(response.headers))
}
return
}
// cloudant stacktrace
+ try {
+ body = await response.json()
+ } catch (e) {
+ body = ''
Review Comment:
or rather, should we make `.json()` vs `.text()` dependent on the response
`Content-Type`?
##########
.github/workflows/ci.yaml:
##########
@@ -22,7 +22,7 @@ jobs:
strategy:
matrix:
- node-version: [14.x, 16.x, 18.x]
Review Comment:
does this not actually work in node 14 or are we just removing this because
it is past LTS?
##########
lib/nano.js:
##########
@@ -15,7 +15,7 @@ const assert = require('assert')
const stream = require('stream')
const Readable = stream.Readable
const undici = require('undici')
-const fetch = global.fetch || undici.fetch
+const fetcher = fetch || global.fetch || undici.fetch
Review Comment:
in what circumstances would `fetch` be false and `global.fetch` truthy?
##########
lib/nano.js:
##########
@@ -15,7 +15,7 @@ const assert = require('assert')
const stream = require('stream')
const Readable = stream.Readable
const undici = require('undici')
-const fetch = global.fetch || undici.fetch
+const fetcher = fetch || global.fetch || undici.fetch
Review Comment:
also, missed opportunity to make a `fetch`, `fetcher`, `the fetchest` pun
##########
lib/nano.js:
##########
@@ -168,29 +163,48 @@ module.exports = exports = function dbScope (cfg) {
delete responseHeaders.server
delete responseHeaders['content-length']
- /* if (opts.dontParse) {
- parsed = body
- } else {
- try { parsed = JSON.parse(body) } catch (err) { parsed = body }
- } */
-
if (statusCode >= 200 && statusCode < 400) {
- log({ err: null, body, headers: responseHeaders })
- if (resolve) {
- resolve(body)
+ // collect response
+ const ct = response.headers.get('content-type')
+ let retval = ''
+ if (ct === 'application/json') {
+ try {
+ retval = await response.json()
+ } catch {
+ // do nothing
+ }
+ } else if (ct && (ct.startsWith('text/') ||
ct.startsWith('multipart/related'))) {
+ retval = await response.text()
+ } else {
+ const ab = await response.arrayBuffer()
+ retval = Buffer.from(ab)
}
- if (callback) {
- callback(null, body, responseHeaders)
+
+ // log
+ log({ err: null, retval, headers: responseHeaders })
+
+ // promisey
+ if (resolve) {
+ resolve(retval)
+ } else if (callback) {
+ // callbacky
+ callback(null, retval, Object.fromEntries(response.headers))
}
return
}
// cloudant stacktrace
+ try {
+ body = await response.json()
+ } catch (e) {
+ body = ''
+ }
+
if (typeof body === 'string') {
body = { message: body }
}
- if (!body.message && (body.reason || body.error)) {
+ if (body && !body.message && (body.reason || body.error)) {
Review Comment:
if yes, we should make that relationship more explicit
##########
.github/workflows/ci.yaml:
##########
@@ -22,7 +22,7 @@ jobs:
strategy:
matrix:
- node-version: [16.x, 18.x, 20.x]
Review Comment:
does this actually not run on 16 or are we just removing it because 16 ist
post-LTS
##########
lib/nano.js:
##########
@@ -15,7 +15,7 @@ const assert = require('assert')
const stream = require('stream')
const Readable = stream.Readable
const undici = require('undici')
-const fetcher = fetch || global.fetch || undici.fetch
+const fetcher = global.fetch || undici.fetch
Review Comment:
hah!
--
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-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
