Use latest nano #342
Use latest nano #342
Conversation
ricellis
left a comment
ricellis
left a comment
There was a problem hiding this comment.
Some initial thoughts on this.
README.md
Outdated
| This feature interfaces with the Cloudant [authorization API][Authorization]. | ||
| Use the authorization feature to generate new API keys to access your data. An API key is basically a username/password pair for granting others access to your data, without giving them the keys to the castle. | ||
| Use the authorization feature to generate new API keys to access your data. An |
There was a problem hiding this comment.
I know this is just a formatting change and a bit unrelated to this PR, but perhaps we should make it more clear here that this is referring to Cloudant's legacy API keys to avoid confusion with IAM API keys.
There was a problem hiding this comment.
Agreed. I'll link to the IBM Cloudant Legacy vs IAM access controls docs.
| See [CORS] for further details. | ||
| ## Virtual Hosts |
There was a problem hiding this comment.
This is deprecated, we can probably remove it for v3, although that should probably happen in another PR.
There was a problem hiding this comment.
+1, I'll raise another PR for this.
|
|
||
| // Specify the database we are going to use (alice)... | ||
| var alice = cloudant.db.use('alice') | ||
| asyncCall().then((data) => { |
There was a problem hiding this comment.
Is it worth pasting these async|promises|both snippets into the readme-examples.js test so we can validate they are working alongside the callback formats?
There was a problem hiding this comment.
I'll rm that legacy/readme-examples.js. It's incomplete. I'll add a new test file containing all code from README.md.
There was a problem hiding this comment.
@smithsz as an FYI there were some necessary changes to that legacy code that I did recently to support parallel running (i.e. using a unique database name instead of alice or test or whatever. Pretty much just adding constants for the DB names instead of using the same string that was used in the examples.
const alice = `nodejs_cloudant_test_${uuid()}`
const my_db = `nodejs_cloudant_test_${uuid()}`| }); | ||
|
|
||
| it('should return a stream', function(done) { | ||
| it('should return a promise', function(done) { |
There was a problem hiding this comment.
This is weird because the whole purpose of the test appears to be to check that we get a stream when we need one. I think we need to test the behaviour of the asStream methods as well if these are now testing promises, unless you think the stream testing elsewhere is sufficient?
There was a problem hiding this comment.
This is all legacy cruft. I just wanted to get it passing. There is no ...AsStream equivalent for the db info func. Perhaps I can use changesAsStream or something instead.
There was a problem hiding this comment.
I don't really want to add tests for the ...AsStream funcs. I think we have enough coverage with the Cloudant client tests:
- client with a callback
- client with event listeners
- client that returns a promise(promise resolve/reject handled by nano) - piping of response body
- piping request payload to a client
There was a problem hiding this comment.
Added legacy stream tests here using cloudant.db.listAsStream func.
smithsz
force-pushed
the
use-latest-nano
branch
from
57b3276 to
ba5709d
Compare
smithsz
force-pushed
the
use-latest-nano
branch
3 times, most recently
from
8b54abb to
4d78dc8
Compare
CHANGES.md
Outdated
| @@ -1,3 +1,8 @@ | |||
| # 3.0.0 UNRELEASED | |||
| - [FIXED] Expose `BasePlugin` type in Cloudant client. | |||
| - [REMOVED] Remove nodejs-cloudant TypeScript type definitions for `db.search`. | |||
There was a problem hiding this comment.
Does this change needs to explain that the Nano search types are used instead?
There was a problem hiding this comment.
Yeah, I'll add some detail.
CHANGES.md
Outdated
| # 3.0.0 UNRELEASED | ||
| - [FIXED] Expose `BasePlugin` type in Cloudant client. | ||
| - [REMOVED] Remove nodejs-cloudant TypeScript type definitions for `db.search`. | ||
| - [UPGRADED] Using nano==7.1.0 dependancy. |
There was a problem hiding this comment.
dependancy -> dependency
Also is it going to be 7.1.0 or do we need something newer with your PRs in?
There was a problem hiding this comment.
Whoops, excuse the typo. Yeah, I'll bump to 7.1.1. That's what's now in package.json.
smithsz
force-pushed
the
use-latest-nano
branch
3 times, most recently
from
ccd25a5 to
5d64701
Compare
smithsz
force-pushed
the
use-latest-nano
branch
from
5d64701 to
5a4850e
Compare
These type definitions are now imported directly from Nano.
smithsz
force-pushed
the
use-latest-nano
branch
from
0c08f6c to
1ec30ae
Compare
2 participants
Checklist
CHANGES.md|CHANGELOG.md) or test/build only changesDescription
Use latest nano dependency.
Fixes #286.
Fixes #289.
Fixes #337.
Fixes #338.
Approach
Removed
promisesplugin logic. Allow Nano to directly pass a promise back to the user.Schema & API Changes
Public API has changed. All functions now return a promise (except for the
...AsStreamfunctions).
Security and Privacy
No change.
Testing
Updated unit tests and removed redundant promise tests.
Monitoring and Logging
No change.