Allowing pouchdb-server to run as middleware!

zevero commented 11 years ago

I have a problem with CORS using pouchdb-server (on port 5984) with my server (on port 3000) serving the latest pouchdb-nightly.js to the browser ( https://github.com/daleharvey/pouchdb/issues/896 )

This leads me to the suggestion to run pouchdb-server without CORS just on the same port as my server. Everything would just be simpler.

So I ask if it would make sense to allow pouchdb-server to be used instead of this:

var server = require("pouchdb-server");
server.listen(5984);

just as middleware like this:

var express = require('express'),
    app = express(),
    pouchdb_server = require("pouchdb-server");

app.use(pouchdb_server.middleware({
   prefix: 'pouchdb'
}));

app.use(whatever...); //my site logic
app.get(whatever....); //my site logic
...
app.listen(3000);

of course this would require for pouchdb-server to return an object to modules.export instead of app... maybe this could be controled by some parameters like

var pouchdb_server = require("pouchdb-server")({middlware: true});

What do you think about this? I could make a fork...

nick-thompson commented 11 years ago

Absolutely, that's a brilliant idea! Do you want to implement it? I'd be happy to accept that PR. If not I can probably get to it sometime next week.

zevero commented 11 years ago

it ended up to be much easier than I thought.

https://github.com/zevero/pouchdb-server/compare/middleware

I changed the Readme in order to show my usage.

Changes in index.js are minimal

removed require('express') as it is now in main app
removed app.use(express.logger) can be done in main app
app = module.exports = express()
removed array from app.all and replaced it with array.forEach (because deprecated)

In my browser (or in node) I connect with

new PouchDB('http://localhost:3000/my_db_prefix/db')

nick-thompson commented 11 years ago

Awesome, that does look pretty easy.

A few things:

removed require('express') as it is now in main app
- https://github.com/zevero/pouchdb-server/compare/middleware#L1R7 ?
removed app.use(express.logger) can be done in main app
- Good call, I like that.
app = module.exports = express()
- Any point to this other than cleanliness? (I would agree that it's cleaner)
removed array from app.all and replaced it with array.forEach (because deprecated)
- Can you point me to the deprecation notice? Haven't seen that before.

After those quick questions I think I'd be pretty ready to merge this if you wanted to open up a PR. There will need to be some tweaks to the bin file, but I can throw those in.

zevero commented 11 years ago

removed require('express') as it is now in main app
* https://github.com/zevero/pouchdb-server/compare/middleware#L1R7 ?
* * I removed it out from package.json as I wanted to use the same version as in the server

It took me quite a while to find out, that you can do app.use('/prefix',anotherapp)!!

Bingo! We can configure two separate apps and bind them together. (Thats why I do require('express')() twice). I know of no other way to do all the routing separably. Otherwise we would have to pass around the app and those prefixes as well.

But there are two issue with binding those apps together:

the logger (when applied twice)
app.use(express.bodyParser()); because it "chokes" on "malformed" requests (thats why there is the costum bodyParser inside pouchdb-server) The solutions I came up with, is either to use it only locally
```
app.post('/some/form', express.bodyParser, function(res, req, next){...
```
or to do
```
app.use(/^((?!\/db).)*$/, express.bodyParser()); //use bodyParser everywhere except for /db route
```

If your version is younger than 4 months, you'll get the deprecation notice on app.VERB(array): https://github.com/visionmedia/express/commit/91c71d6c2ec2e39bf9f11be79494f76c83bec4c5

Then there is my useCorser boolean! Setting it to true works as nicely. It was just my pride of not needing it anymore, which set it to false :) You see, I never understood CORS and the security policy! When I have a server and offer a service it should be my responsability to state, whereto my site is eligible to connect. But CORS seems to me to be the other way around! Some other site declares via access-control-allow-origin if it wants to be contacted from a browser at my site? While at the same time being perfectly reachable from the whole world??? I really dont get it! Do you?

daleharvey commented 11 years ago

CORS is done exactly as you said, the server provides Allow-Origins and other headers to say who can connect and what site from and the client (browser) checks those assumptions before it does any operations. It get confusing because sometimes the client has to precheck things (OPTIONS requests)

daleharvey commented 11 years ago

Also this is awesome, thanks

zevero commented 11 years ago

Offtopic Cors: Yes, when Allow-Origins are strictly set, the browser may refuse the connection. But this connection could always be proxied via the server, right? If so - this makes no sense to me.

nick-thompson commented 11 years ago

Thanks zevero!

Yea, you could write a client that talks to your own server which then proxies a remote server to get around the need for CORS. Pretty sure it's just that servers aren't bound by the Same Origin Policy, as browsers are.

Just a few more things before I'm ready to merge:

I definitely agree that the version of Express should be the same in the main server as in the middleware, but we won't be able to just take the dependency out of the package.json because some users will choose to run pouchdb-server as a standalone. We'll want to expose pouchdb-server both as standalone and as middleware... I'll have to look into NPM dependencies to see how we can handle that.
Took me a second to see where you were going with the useCorser boolean. I agree with the idea, although I think we should expose it slightly differently. Perhaps a function that the user can call to enable the CORS functionality:

// To run the server standalone:
require('pouchb-server').enableCORS().listen(300);

// Or to require it as middleware (does not enable CORS)
app.use('/prefix', require('pouchdb-server'));

What do you think?

zevero commented 11 years ago

https://github.com/nick-thompson/pouchdb-server/pull/21

Please look into the Readme.md for my proposed usage. In standalone mode the express version of the dependency is taken In server mode I pass the express variable to the module.

nick-thompson commented 11 years ago

Hey @zevero, sorry about the slow reply, was out of the country on vacation the past few days.

So I've been thinking about this and exploring the idea of mounting express apps more in general, and I think I might be leaning towards restructuring pouchdb-server on the whole to support this in the best possible way. Sorry to be changing my mind again, but first watch this video then tell me what you think about the following:

Restructure pouchdb-server to be only a partially functional express app which expects to be mounted into another application (kind of like a plugin). This would allow for app.use('/_prefix', require('pouchdb-server')); without any problems or any additional configuration. pouchdb-server will no longer include any sort of binary executable to run in a standalone mode as it currently does. The reason for this is that we can then drop the dependency on Express in pouchdb-server completely, and rather define a peerDependency. This is mostly why the app will be partially functional - it expects that you will mount it into a separate app which already has Express included (thereby eliminating competing versions).

That restructuring seems to make the most sense in terms of allowing pouchdb-server to be mounted into other Express apps. Then, to address the standalone execution, I'm proposing a separate NPM module, pouchdb-server-cli, which very simply requires pouchdb-server and gives it a simple CORS-enabled Express host to be run in standalone mode from the terminal (basically what we have now).

The more I think about it, the more I like this idea that you've proposed, and I really want to make it happen in the best possible way. So, again, please excuse me for changing my mind, and tell me what you think about this proposal (and @daleharvey if I could get your opinion too that'd be awesome).

P.S. - This would involve kind of a lot of code refactoring, but I'm happy to work on it. I have a bunch of time today and the next few days in which I could get it done, and I've been wanting to rework the documentation anyway.

zevero commented 11 years ago

Yeah, this is exactly the video, where I got that idea from to app.use(anotherapp)

I don't think, that it is nice to have differnt npm-repositories because other than the express dependency and three lines of code, nothing would differ.

Please have a look at my pull request https://github.com/nick-thompson/pouchdb-server/pull/21 It resolves all issues an has a nice interface.

The express dependency doesnt hurt, since it is only used in standalone mode. In middleware mode the right version is passed.

Another option is to drop standalone mode as a whole, because in the current state you need anywhay to require pouchdb-server. It is only one line more to add require('express'); and a note in Readme.md would be sufficient.

nick-thompson commented 11 years ago

Yeah, this is exactly the video, where I got that idea from to app.use(anotherapp)

Ah, cool :)

Another option is to drop standalone mode as a whole, because in the current state you need anywhay to require pouchdb-server. It is only one line more to add require('express'); and a note in Readme.md would be sufficient.

Sorry, not quite what I meant; one of the nice features of pouchbd-server is being able to run it from the command line really easily. That's what the ./bin/pouch file is for: https://github.com/nick-thompson/pouchdb-server/blob/master/package.json#L15

I totally understand your point, and I see where you're coming from. But, if my understanding is correct, this situation is a big reason for why Grunt split out into two projects in version 0.4 and specifically why NPM implemented peerDependencies. I agree that this could be done within a single repository but my feeling is that that might not be best way to do it.

Hm... I'm going to read a little more into all of this.

zevero commented 11 years ago

I am afraid, but I don't understand the trouble, as in my pull request the the api and behaviour has not changed at all (because my module.exports.listen() calls app.listen() )

So concerning bin/pouch the only change necessary was to insert useCORS(true), which could be made an argument. Maybe we should default useCORS to true anyway, so the user wont have problems. https://github.com/zevero/pouchdb-server/commit/3fbbcfcad26164861da5fe93e5ef275f7ca5e612

The dependency for express is necessary for bin and standalone mode. In middleware-mode it is just not used. I don't see it as a problem to have an unused directory in node_modules (its the same with corser anyway).

nick-thompson commented 11 years ago

I am afraid, but I don't understand the trouble, as in my pull request the the api and behaviour has not changed at all (because my module.exports.listen() calls app.listen() )

Right, I agree, your code is fine and everything still works as expected. The main point here is that, in light of your suggestion, I think the most sensible solution would be to factor out the code in pouchdb-server which could be used as an express plugin. It's just a better division of the codebase and cleaner solution overall, in my opinion.

I had a chat with @daleharvey earlier today in #pouchdb on Freenode IRC, and he seemed to agree with that opinion. So, I'm going to startup a new project and start refactoring the code into that new project just to more clearly realize this solution.

I'm going to leave your PR and this issue open for the time being, and I'll pop back in when I've made a little progress!

nick-thompson commented 11 years ago

@zevero take a look at this: https://github.com/nick-thompson/express-pouchdb

I refactored all of the important parts out of pouchdb-server in a very express-friendly way. Now express-pouchdb can be used exactly as you had asked for with extremely little setup. I'm working on refactoring pouchdb-server to simply wrap and expose express-pouchdb as a standalone server. Check out the refactor branch if you want to see the progress.

Let me know what you think.

zevero commented 11 years ago

Hi @nick-thompson

Now I can see what you were up to. Yes! This is much cleaner! CORS is only needed in standalone mode and only there it should go. Very nice!

A last remark about naming: Since the project is called pouchdb-server (for standalone), I would prefer the rather longish name express-pouchdb-server (for middleware) in order to not get confused with the pouchdb client.

nick-thompson commented 11 years ago

Glad you like it :)

I'm going to go ahead and close this issue for now then. If you have any other ideas/suggestions feel free to reopen this or open new issues either here or on express-pouchdb.

Also, good point about the naming; I'm going to ask around a bit and see if it's confusing people.

Thanks zevero!

pouchdb / pouchdb-server

Allowing pouchdb-server to run as middleware! #18