merge pouchdb-hoodie-api and pouchdb-hoodie-sync #153
Conversation
|
this PR is ready for review @hoodiehq/maintainers 👀 |
lib/helpers/add-many.js
Outdated
| @@ -0,0 +1,38 @@ | |||
| var utils = require('pouchdb-utils') | |||
There was a problem hiding this comment.
pouchdb-utils is missing in the dependencies.
This may also be the related to why this test is failing for me, because require('pouchdb-utils').extend is undefined (it seems pouchdb-utils doesn't export this method anymore https://github.com/pouchdb/pouchdb/releases/tag/6.1.1). Using pouchdb-utils@5 does work though.
| @@ -0,0 +1,42 @@ | |||
| var PouchDBErrors = require('pouchdb-errors') | |||
There was a problem hiding this comment.
pouchdb-errors is also missing in the dependencies. See my comment from above.
tests/integration/reset.js
Outdated
| .catch(t.error) | ||
| }) | ||
|
|
||
| test.only('events work on scoped APIs after reset', function (t) { |
There was a problem hiding this comment.
I think .only should be removed
|
@flootr do still want to give it another look? |
|
@gr2m Yes, I'd love to do this in the evening (cet) today if that's okay. Had no time during the weekend unfortunately :( |
|
no problem, don’t feel pressured, it’s all good :) If you could have a look tonight that’d be great |
There was a problem hiding this comment.
@gr2m I've reviewed again and had a deeper look mostly coming up with questions, but spotted nothing serious. This PR is rather huge so it may be sensible that another person more familiar with the code base (and the tests) should have a look. Nevertheless it was fun 👍
| * adds one or multiple objects to local database | ||
| * | ||
| * @param {String} prefix optional id prefix | ||
| * @param {Object|Object[]} properties Properties of one or |
There was a problem hiding this comment.
You named it properties but the param is named objects. Should we choose the same term to prevent confusion?
| */ | ||
| function disconnect (state) { | ||
| if (state.replication) { | ||
| state.replication.cancel() |
There was a problem hiding this comment.
Afaik .cancel() is finished when the complete event is emitted by PouchDB. Is it sensible to wait for this event before resolving and/or emitting the disconnect event?
There was a problem hiding this comment.
Great catch! This didn’t cause problems so far, so I don’t think it’s critical, but I created a follow up issues for it so we don’t forget to fix it: #155
lib/find-or-add.js
Outdated
| * with passed properties. | ||
| * | ||
| * @param {String} prefix optional id prefix | ||
| * @param {String|Object} idOrObject id or object with `._id` property |
There was a problem hiding this comment.
The function param says it also accepts an Array so might we extend this description?
lib/find-or-add.js
Outdated
| * | ||
| * @param {String} prefix optional id prefix | ||
| * @param {String|Object} idOrObject id or object with `._id` property | ||
| * @param {Object} [properties] Optional properties if id passed |
There was a problem hiding this comment.
I'm a bit confused because the param is named newObject - if I'm not mistaken - in the function. Is it relevant to rename it to have the same term?
lib/find.js
Outdated
| * finds existing object in local database | ||
| * | ||
| * @param {String} prefix optional id prefix | ||
| * @param {String|Object} idOrObject Id of object or object with |
There was a problem hiding this comment.
The function accepts also an Array so should we extend the description here?
|
|
||
| .catch(function (error) { | ||
| if (error.status === 404) { | ||
| var missing = new Error('Object with id "' + id + '" is missing') |
There was a problem hiding this comment.
Regarding my comment about error construction. Might this also be sensible to extract as a helper function if we need it at different places?
lib/update.js
Outdated
| * updates existing object. | ||
| * | ||
| * @param {String} prefix optional id prefix | ||
| * @param {String|Object} idOrObject id or object with `._id` property |
There was a problem hiding this comment.
I think we should cover that idOrObject/obectsOrIds also accept Arrays and unify naming, shouldn't we?
lib/utils/isnt-design-doc.js
Outdated
| @@ -0,0 +1,4 @@ | |||
| // Checks for a design doc, so we can filters out docs that shouldn't return in *All methods | |||
| module.exports = function isntDesignDoc (row) { | |||
| return row.id.match(/^_design/) === null | |||
There was a problem hiding this comment.
Just a side note but I think return /^_design/.test(row.id) would be slightly faster (depending on the browser).
There was a problem hiding this comment.
that would make a great follow-up PR 👍 do you want to send one? Thanks for checking it!
lib/with-id-prefix.js
Outdated
| * | ||
| * @param {String} prefix String all ID’s are implicitly prefixed or | ||
| * expected to be prefixed with. | ||
| * @return {Promise} |
There was a problem hiding this comment.
As far as I can see this method is not returing a Promise.
|
|
||
| if (prefix) { | ||
| options.startkey = prefix | ||
| options.endkey = prefix + '\uffff' |
There was a problem hiding this comment.
What is this \uffff character?
There was a problem hiding this comment.
it’s the highest Unicode character, so it makes sure it gives us all keys which start with the prefix string, see http://docs.couchdb.org/en/2.0.0/couchapp/views/collation.html#string-ranges (I don’t know why they use ufff0 instead of uffff in the example though)
| } | ||
| ) | ||
| return state.db.destroy() | ||
| }) |
There was a problem hiding this comment.
gosh this change alone makes it all worth it already :D
|
this has been open long enough and I gave it thorough reviews myself, so merging it in now |
They have beeen removed via hoodiehq/hoodie-store-client#153
closes hoodiehq/camp#121, closes #149