Promise support for Nano - Issue 98 #102

glynnbird · 2018-07-13T16:06:08Z

Overview

Aimed at a v7.x release of Nano, this PR makes Nano return a Promise instead of a Request object as its return value. See #98. This brings Nano into the modern world without breaking code that uses the callback style. It only breaks code that relied on the events emitted from the Request object or that piped the stream to another stream.

Work done in this PR:

most functions return a Promise
new ...AsStream| functions are availble for operations that return lots of data. These return a Request object
test suite updated
README changed to reflect the new API
TypeScript definitions updated too
written migration_6_to_7 guide

Testing recommendations

The tests have been updated to reflect the change. Instantiate a Nano instance and most operations will return a Promise:

var nano = require('.')
var n = nano('http://localhost:5984')
var db = n.db.use('animals')
db.list({start_key:'cat1',end_key:'cat4'})
  .then(console.log)
  .catch(console.error)

GitHub issue number

#98

Related Pull Requests

#88

Checklist

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

garrensmith

This is great work. Just some smaller changes and then I think this is good to go.

garrensmith · 2018-07-18T09:02:22Z

lib/nano.d.ts

+
+    // Do not wait for the index to finish building to return results.
+    stale?: boolean
+  }


Wow nice work. The definitions are nice and clear now.

garrensmith · 2018-07-18T09:04:42Z

lib/nano.js

+  var responseHandler = function(req, opts, resolve, reject, callback) {
+
+    return function(e, h, b) {
+      var parsed;


Since we only supporting more modern nodejs versions. You can use let for var. But ideally use a const. So when you define an object use const.

I wasn't sure how far to go with the modernisation of the code. If we're going to use const here, then I'll take a look at doing const/let everywhere else too.

Well maybe do that in a separate PR.
Lets get this landed. Then do a modernisation and then do a release for 7.0.0

garrensmith · 2018-07-18T09:06:58Z

lib/nano.js

+
+    return function(e, h, b) {
+      var parsed;
+      var rh = h && h.headers || {};


I think this can be simplified to:

const responseHeader = Object.assign({ uri: req.uri, statusCode: h.statusCode }, h.headers);

I'm not a huge fan of such short variable names. Its difficult to figure out what it is. I would before something that explains what this is.

👍 This e/h/b business is historic. I've refactored this function to aid readability.

garrensmith · 2018-07-18T09:08:26Z

lib/nano.js

@@ -68,6 +67,89 @@ module.exports = exports = nano = function dbScope(cfg) {
    }
    return str;
  }
+  var responseHandler = function(req, opts, resolve, reject, callback) {
+
+    return function(e, h, b) {


You do a fiew checks lower to check if h exists. You can do something like this instead:

return function (e, h = {}, b) {

That way h will default to an empty header. Also could you rename it to header because I think that is what it is.

👍 h is actually the response!! h is from the original code. I'm refactoring with sensible names!

garrensmith · 2018-07-18T09:09:10Z

lib/nano.js

+      rh.uri = req.uri;
+      if (e) {
+        log({err: 'socket', body: b, headers: rh});
+        var ret_e = errs.merge(e, {


I think we using camel case so this should be const retE or `returnError

garrensmith · 2018-07-18T09:15:23Z

lib/nano.js

+        }, function(err, body, headers) {
+          if (err) {
+            if (callback) {
+              callback(err);


Would there be a case you want the body and headers for an error as well?

Good point 👍

garrensmith · 2018-07-18T09:17:01Z

lib/nano.js

+      return view(ddoc, viewName, {type: 'view', stream: true}, qs, callback);
+    }
+
+
    // geocouch
    function viewSpatial(ddoc, viewName, qs, callback) {


We might have ton consider dropping support for Geocouch. It doesn't work with CouchDb 2.x

I'll create an issue to track this.#103

garrensmith · 2018-07-18T09:20:52Z

lib/nano.js

@@ -738,6 +825,25 @@ module.exports = exports = nano = function dbScope(cfg) {
      }, callback);
    }

+    function insertAttAsStream(docName, attName, att, contentType, qs, callback) {
+      if (typeof qs === 'function') {


we have this check everywhere. You could consider a function that does this everywhere for us.

const {qs, callback} = getCallback(qs0, callback0); function getCallback (opts, callback) { if (typeof opts === 'function') { callback = opts; qs = {}; } return { qs, callback }; }

It might make things a little neater. I know we do something similar in PouchDB.

garrensmith · 2018-07-18T09:21:59Z

migration_6_to_7.md

+
+```js
+// Nano 7
+var db = nano.db.use('mydb')


lets promote good practice and use const here

garrensmith · 2018-07-18T09:22:33Z

migration_6_to_7.md

+```js
+// Nano 7
+var db = nano.db.use('mydb')
+var x = db.get('mydoc') 


You could maybe make this const x = await db.get('mydoc'); To show its a promise and we could use async/await

janl · 2018-07-22T20:13:06Z

.jshintrc

@@ -3,11 +3,12 @@
  "node":      true,
  "strict":    true,
  "smarttabs": true,
-  "maxlen":    80,
+  "maxlen":    100,


janl

nicely done @glynnbird! I gave it a very shot spin in a local project and it seems to work just fine.

janl · 2018-07-22T20:13:53Z

migration_6_to_7.md

+```js
+// Nano 7
+const db = nano.db.use('mydb')
+const x = await db.get('mydoc') 


x is a what now? (just so the examples have parity)

janl · 2018-07-23T12:58:25Z

migration_6_to_7.md

+})
+```
+
+The Promise style of code lets you code sequences of asynchronou calls at the same level of indentation (the callback style forces you to indent further to the right with each call!):


s/asynchrnonou/asynchrnonous/

janl · 2018-07-23T13:04:32Z

README.md

+})
+```
+
+or in the `async/await` style:


nickpick, but should we make this the first example and show raw promises second? The code is just so much nicer :)

janl · 2018-07-23T13:11:40Z

@glynnbird how does this relate to #88?

glynnbird · 2018-07-23T13:20:09Z

@janl The idea of #88 was to make it so that Promises could be switched on with a config flag. A neat idea, but it plays havoc when you're using it with TypeScript which likes functions to return a fixed type, so instead I wrote-up issue #98 to make Promises the default - to mirror the expectations of the modern JS/TS user. That way the return values would always be Promises, which keeps TS happy and added some extra functions which return streams to the streamy stuff. So, in short, #88 was the inspiration, but it slightly different approach won the day.

janl · 2018-07-24T09:41:03Z

@glynnbird thanks for the context!

garrensmith

This is awesome. Thanks @glynnbird huge +1

garrensmith · 2018-07-24T11:03:49Z

lib/nano.js

+
+    return function(err, response = { statusCode: 500 }, body = '') {
+      let parsed;
+      const responseHeaders = Object.assign({


Thanks this is much clearer to read.

glynnbird added 27 commits July 2, 2018 15:39

package-lock.json audit fix

9a2c6be

return Promise by default

9da2b52

tests passing

09e01b5

amend integration/document tests wrt Promise return values

bd7c28d

amend integration/attachment tests wrt Promise return values

8df3fac

amend integration/database tests wrt Promise return values

c24f144

amend integration/design tests wrt Promise return values

461e4d8

amend integration/multipart tests wrt Promise return values

3e6a6e2

amend integration/shared tests wrt Promise return values

5fcbcdf

amend integration/util tests wrt Promise return values

28cbdab

amend intercept/design tests wrt Promise return values

f86accd

readme rewrite

9a2b10b

first stab at TypeScript definition returning Promises

44f181a

added ...AsStream varaiants

645daa0

add TypeScript to the README

d35494e

test fix

a4f8ba2

find query missing from README

88eeee9

added findAsStream and find tests

a4e6dde

added searchAsStream

b3013a0

searchAsStream README

cc5807d

add viewAsStream

f4566df

added changesAsStream

7127f26

fix tests

bfb4e06

add migration guide from nano 6 to nano 7

5a47afe

tests and jshint work

5bc1e1c

99+% test coverage

56bc512

migration guide

e537418

glynnbird requested a review from garrensmith July 13, 2018 16:08

garrensmith requested changes Jul 18, 2018

View reviewed changes

swap out var for const/let in the core code and tests

dc7687d

glynnbird added 7 commits July 18, 2018 13:22

refactor responseHandler to make parameter names more readable

6eac47d

add default parameter value in responseHandler

2e84992

return headers for headDoc on an error

2d3b22c

added central getCallback function to remove code duplication

11c6168

var--->const in the README

214a0da

modernise JS in migration guide

166c23c

jshint fixes

abab887

janl reviewed Jul 22, 2018

View reviewed changes

janl approved these changes Jul 23, 2018

View reviewed changes

glynnbird added 2 commits July 23, 2018 14:29

further documentation changes after feedback from @janl

155c139

use official CouchDB docker image at the suggestion of @wohali

1d5fd35

garrensmith approved these changes Jul 24, 2018

View reviewed changes

glynnbird merged commit 8d71498 into master Jul 24, 2018

glynnbird deleted the issue98 branch July 24, 2018 11:19

Promise support for Nano - Issue 98 #102

Promise support for Nano - Issue 98 #102

Conversation

glynnbird commented Jul 13, 2018 • edited by janl Loading

Overview

Testing recommendations

GitHub issue number

Related Pull Requests

Checklist

garrensmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janl commented Jul 23, 2018

glynnbird commented Jul 23, 2018

janl commented Jul 24, 2018

garrensmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glynnbird commented Jul 13, 2018 •

edited by janl

Loading