1999 / node-couchdb

ES2015-compatible package to interact with CouchDB
76 stars 32 forks source link

standardize results #21

Open Flamenco opened 7 years ago

Flamenco commented 7 years ago

They are all docs.

Furthermore, for mango and view, I need to check the length to find out if there were any results.

if (response.data.rows.length > 0){
   response.data.rows.forEach(row=>{})
}

I would like to simply code this (in all cases):

if (response.hasDocs(){
   response.getDocs().forEach(doc=>{});
}

or even better

response.getDocs().forEach(doc=>{});

or even better

response.forEachDoc(doc=>{})

Any thoughts on shoving a forEachDoc interface into the response object before calling resolve on the promise?

1999 commented 7 years ago

I would say that this is a bit complicated. Why couldn't you just use this: response.data.rows.forEach(row => {...}) or just this: for (let row of response.data.rows) {...}. forEachDoc is too ambigous for iteration here, simple JavaScript better especially if node-couchdb is using arrays (response.docs) for data.

Flamenco commented 7 years ago

The reason for this is that I have 3 or more ways to query a document. They each return the exact same results in a different format. The document is sometimes in response.data.rows, sometimes in response.data.docs, and sometimes it a singleton in response.data. I would say that is a bit complicated.

node-couchdb has made my code much more succinct, and this suggestion certainly helps to further that.

Anyway, I wrote a library to handle this and am going to publish to NPM shortly. There is a ctor that wraps node-couchdb; My PRs have been addressing couchdb internals so I can avoid have to polyfill the library or have users require my fork. My project also handles web socket streaming the _changes endpoint, but that also has a bunch of internal node-couchdb enhancements.

1999 commented 7 years ago

I have 3 or more ways to query a document

Can we start from here. Post your example please.

Flamenco commented 7 years ago

This is the pattern I decided upon. 12 lines of source cut 2000 lines of code from my project!

function getComponentData() {
    return new Promise((resolve, reject) => {
        medusaConnector.newBuilder()
            .mango({
                selector: {
                    type: 'usage'
                },
                limit: 10000
            })
            .then_docs(resolve)
            .catch(reject)
    });
}

function getComponentData() {
    return new Promise((resolve, reject) => {
        medusaConnector.newBuilder()
            .list('default', 'myList')
            .then_docs(resolve)
            .catch(reject)
    });
}

function getComponentData() {
    return new Promise((resolve, reject) => {
        medusaConnector.newBuilder()
            .get('1234)
            .then_docs(resolve)
            .catch(reject)
    });
}

An array is always returned, and is empty if no results were found.

There is also a then_doc function that returns a single doc, the first doc, or null.

function getComponentData() {
    return new Promise((resolve, reject) => {
        medusaConnector.newBuilder()
            .mango({
                selector: {
                    type: 'usage'
                },
                limit: 1
            })
            .then_doc(resolve)
            .catch(reject)
    });
}

function getComponentData() {
    return new Promise((resolve, reject) => {
        medusaConnector.newBuilder()
            .list('default', 'myList')
            .then_doc(resolve)
            .catch(reject)
    });
}

function getComponentData() {
    return new Promise((resolve, reject) => {
        medusaConnector.newBuilder()
            .get('1234)
            .then_doc(resolve)
            .catch(reject)
    });
}
1999 commented 7 years ago

An array is always returned, and is empty if no results were found. There is also a then_doc function that returns a single doc, the first doc, or null.

Well now if you call get(dbName, documentId) you get either document as a promise result or EDOCMISSING rejected promise. When you call get(dbName, viewName, query) you get array as a promise result or EDOCMISSING if this design document / view name doesn't exist. I cant agree that returning array for both of these requests is a good API for this method. We can separate this method into "getDocument" and "getView" instead. What do you think about this?

Flamenco commented 7 years ago

I think that whether I call getView, getDocument, or getMango, I should be able to request getting a single document or a collection. This is how JPA works, and it is very standard. You ask for the query results as a list or a single document. This way you don't have to check the returned object type and you can pipe it into functions that expect the result shape.

In the case of the missing view, reject will still be called. But if it does exist, no results will return null for then_doc() and an empty array for then_docs().

If you are concerned that your single document request does not exist, and you need to handle that differently, you can still call the standard methods. This implementation returns null, and that makes it very easy to check in the resolve function.

Let me know if I am missing something here.

1999 commented 7 years ago

Hm... What do you think of this kind of implementation:

class PromiseWithThenDoc extends Promise {
  thenDoc() {
    // your logic here
  }
}

And return this kind of promise from each method? I can also give you an example of this.

Flamenco commented 7 years ago

That works.

With the current implementation, you need to call then() to kick off the action get the promise, yes? To work around that in our builder pattern, we added an exec() method that returns the promise. I call then() or thenDoc() on that returned object.

exec().then(...)
exec().then_docs(...)

Here's my source from our current solution. Instead of superclass, we delegate the promise to your implementation, filter the results, and resolve the wrapper.

 then_docs() {
        const self = this;
        return new Promise((resolve, reject) => {
            self.then((res) => {
                if (res.data.rows) {
                    const docs = res.data.rows.map(row => row.doc ? row.doc : row.value).filter(doc => doc != undefined);
                    resolve(docs);
                } else if (res.data.docs) {
                    resolve(res.data.docs);
                } else {
                    resolve([res.data]);
                }
            }, reject);
        });
    }

then_doc has a similar approach.

What do you think? This has no effect on existing code, and is very DRY.

1999 commented 7 years ago

Go ahead with PR. The only thing is that new Promise isn't needed here:


// let it be thenDocs please
thenDocs(...args) {
  return this.get(...args).then(res => {
    if (res.data.rows) {
      // return ...
    } else { ... }
  });
}
Flamenco commented 7 years ago

I'm OK with the suggested naming. I will try it without the promise in our use case and see how it works, although I don't see in the last example how a resolve function is passed into thenDocs().

This is how we call it.

exec().thenDocs().thenProcessDocs().thenShowDocs().catch(...)

The nice thing is that we can switch between views, single docs, _all_docs, and mango without changing anything.

For the record, one other approach is to add a filter to the current implementation and just keep things the same.

then().getDocs().thenProcessDocs().thenShowDocs().catch(...)
1999 commented 7 years ago

I don't see in the last example how a resolve function is passed into thenDocs().

What's the point of it? thenDocs() should just return promise which is resolved with array, so we can go without separate new Promise here. PR from you would be great, because I would be able to show you this right on your example.

Flamenco commented 7 years ago

then() returns the standard response from node-couchdb, getDocs() normalizes the response using my previous example. Basically separating the query from the normalize logic.

Sorry, I should have written it as: then(getDocs).then(processDocs).then(showDocs).catch(...)

1999 commented 7 years ago

Okay. I thought that your point was to integrate getDocs/thenDocs into node-couchdb?.. Again, PR would be great, because currently we're discussing non-existing code.