added Koa strategy - Githubissues

shannon commented 8 years ago

Added a strategy for Koa. It was pretty close to the express one with a few minor tweaks here and there since Koa merges req and res into one ctx for convenience.

Solves #37

ethanresnick commented 8 years ago

This looks awesome!

I'm going to go through it and ask some questions inline (apologies if some of them are stupid—I don't know Koa well), but it looks great.

Thanks!

shannon commented 8 years ago

@ethanresnick So I made some changes. The minor ones you suggested and I decided to make a new function called prepareRequest. It makes the Koa req object more like the Express req object. There were too many little differences (E.g. somethings missing from req, ctx not behaving like a stream). So I thought this would be the cleanest most explicit approach.

I also updated the Koa example. For that there was enough difference from the Express example that I just made the whole thing instead of trying to explain the differences. Just so there is no confusion.

Oh and the build file was missing so I added that.

I'm happy to make any more changes.

ethanresnick commented 8 years ago

Thanks for the updates @shannonmpoole!

A few high-level things:

Looking at how much this has in common with the Express strategy, which I know is by design, maybe it makes more sense to create a BaseStrategy that both the KoaStrategy and ExpressStrategy could subclass? That seems like it would be much more DRY, and would save us from needing to apply bug fixes in both places. If that makes sense to you, are you up for taking a crack at it?
Don't worry about the build files. I actually prefer to take PRs without them (it makes the code easier to review), and then they're rebuilt automatically each time I publish a new version of the module to NPM.
For the query string stuff, is it possible/would it make sense for the adapter to register the koa-qs middleware (which would then become a peerDependency for this library, like mongoose/express). This isn't necessary exactly, but I thought, if it's (almost) always going to be required, it might be nice to save the users from having to configure it themselves.

shannon commented 8 years ago

@ethanresnick

Yea I got to a point were I was thinking it might make sense to add buildRequestObject to the Express class and then just extend it but I think a base class would be better. I can probably handle it. Do you think it makes sense to just do a different pull request? This one is going to be pretty different anyways. No sense in merging this the way it is. Plus I can just leave the build file out this time around.

For the koa-qs peer dependency I'm a little unclear on what this would look like. koa-qs requires the app to be passed into it. It's not normal middleware which is unfortunate. It seems to be just assigning a get/setter on the app.request.query property so maybe it could just be applied to the ctx. I'd have to try it out. I'm not sure we want to require the app as another parameter to the strategy. I tend to keep my routes in another file so passing the app around would be kind of a pain. I do like the idea of the strategy doing it automatically though.

So I probably won't be able to get to start on the base class until tomorrow evening EST.

ethanresnick commented 8 years ago

Cool!

Do you think it makes sense to just do a different pull request?

I think it's fine to just rebase this one, since the final PR will still add a Koa strategy. (That way there aren't two PRs for the same thing when people search.)

I'd have to try it out. I'm not sure we want to require the app as another parameter to the strategy. I tend to keep my routes in another file so passing the app around would be kind of a pain. I do like the idea of the strategy doing it automatically though.

Yeah. Play with it and lmk what you come up with, but ultimately I trust your judgment.

So I probably won't be able to get to start on the base class until tomorrow evening EST.

That's fine!

shannon commented 8 years ago

@ethanresnick I've implemented the base strategy. I figured since most http frameworks are built on top of the core http module I should build buildRequestObect around that. Both Koa and Express expose it, well Express just extends it. The only thing I couldn't get from the IncomingMessage object was the protocol and the url params(this is obviously dependent on the framework). So those are arguments passed in as well. And the last argument is an optional query parameter. If you leave this off it runs the query string through the qs module. This means we don't need users to configure koa-qs. It's an added dependency but it's pretty small.

Full disclosure: I've been pretty sick this weekend so I'm a bit foggy, hopefully the code doesn't reflect that. I'm not sure why it's failing the circleci check though.

ethanresnick commented 8 years ago

I figured since most http frameworks are built on top of the core http module I should build buildRequestObect around that.

:+1: Makes a lot of sense.

The only thing I couldn't get from the IncomingMessage object was the protocol and the url params(this is obviously dependent on the framework).

Url Params makes sense, though I'm a bit surprised about protocol. It looks like express gets it by accessing req.connection, which returns the socket that the request is associated with. Then you can check .encrpyted to distinguish http/https.

Looking at express's implementation raises an issue, though, which is whether to trust the X-Forwarded-Proto header. Since we probably don't want to duplicate Express's logic for that (which involves user config settings), I think we should still allow protocol to be passed into buildRequestObject. But, if it isn't passed in, switching on req.connection.encrypted and ignoring X-Forwarded-Proto would be a good default.

This same issue actually applies to Host too. That is, we can't just read req.headers.host, because that might be overriden by X-Forwarded-Host (which we might or might not want to allow). So, I'd likewise allow host to be passed in, with the fallback being req.headers.host.

And the last argument is an optional query parameter. If you leave this off it runs the query string through the qs module. This means we don't need users to configure koa-qs. It's an added dependency but it's pretty small.

Makes sense! I have a small comment about the implementation, which I'll leave in line, but I like that you got this working without requiring anything from the user!

Full disclosure: I've been pretty sick this weekend so I'm a bit foggy, hopefully the code doesn't reflect that.

Feel better! And no, the code is good :)

I'm not sure why it's failing the circleci check though.

Yeah, sometimes generating the code coverage fails, for reasons I can't yet ascertain. Could be circleci's fault, or could be something with how I've configured it. Either way, the tests are passing so it's not a problem with your code.

shannon commented 8 years ago

@ethanresnick sorry it took me so long to get to this. I think I am fully recovered now. I added the changes you requested. The only thing I didn't do was:

Makes sense! I have a small comment about the implementation, which I'll leave in line

I didn't see any inline comment so I assume it's still to come.

ethanresnick commented 8 years ago

This looks beautiful! Merged :)

Don't worry about inline comment I was going to leave; it's not relevant, actually.

(Btw, the tests aren't passing because of this, which is annoying.)

ethanresnick commented 8 years ago

This is up on npm too now, btw :)

shannon commented 8 years ago

Perfect thanks :-)

ethanresnick / json-api

added Koa strategy #94