Support Koa 2.0 #47
Support Koa 2.0 #47
Conversation
lib/index.js
Outdated
| pretty: false, | ||
| compileDebug: false, | ||
| locals: {}, | ||
| basedir: `${__dirname}/templates`, |
There was a problem hiding this comment.
note that this means that client templates are not resolvable currently -- we'll need to fix this before we merge.
|
Wow, nice work, thanks! I'll try to do a detailed review tonight or tomorrow. |
| @@ -3,3 +3,4 @@ node_modules | |||
| coverage | |||
| cache | |||
| *.log | |||
| *.out | |||
There was a problem hiding this comment.
I do -- when I'm debugging, I pipe output to out files, like npm test | tee test.out
README.md
Outdated
| @@ -12,14 +12,6 @@ A suite of Koa utilities allowing for quicker bootstrapping, | |||
| as well as a consequential guide on how to write apps using the Koa philosophy. | |||
| Think of it as a KrakenJS for Koa. | |||
|
|
|||
| ## Status | |||
|
|
|||
| Beware! Koala is alpha software! | |||
There was a problem hiding this comment.
I think it would help to leave the alpha warning, unless you'd like this to go into a stable release before we publish another unstable release.
README.md
Outdated
| Beware! Koala is alpha software! | ||
|
|
||
| [Koa 2](https://github.com/koajs/koala/issues/17) is currently | ||
| unsupported. Please refer to [Contributing](CONTRIBUTING.md) if you'd like to |
There was a problem hiding this comment.
Let's also remove this from contributing, unless if we have other Koa 2 related issues after merging this pull request.
lib/middleware/index.js
Outdated
|
|
||
| module.exports = function (app, options) { | ||
| options = options || {}; | ||
|
|
||
| // methods | ||
| if (options.responseTime !== false) app.use(require('koa-response-time')()); | ||
| app.use(require('./trace')); | ||
| // app.use(require('./trace')); |
There was a problem hiding this comment.
Did you want to use koa-trace for this? I noticed you have a fork of it.
package.json
Outdated
| "supertest": "^2.0.0" | ||
| "istanbul-harmony": "0", | ||
| "mocha": "^3.5.0", | ||
| "supertest": "^3.0.0" |
There was a problem hiding this comment.
Sweet, thanks for fixing the supertest update. I also like that we're using more ^ versions here.
package.json
Outdated
| "istanbul": "^0.4.5", | ||
| "mocha": "3", | ||
| "supertest": "^2.0.0" | ||
| "istanbul-harmony": "0", |
There was a problem hiding this comment.
istanbul-harmony was deprecated in favor of istanbul@>=0.4.0. Let's revert the first line of devDependencies please.
There was a problem hiding this comment.
Whoops, I didn't realize the new syntax is broken on that version of Istanbul. I would like to eventually switch to their new CLI, https://github.com/istanbuljs/nyc, which I believe solves this problem. Either way I noticed this is fixed on instanbul@v1.1.0-alpha.1 but I feel weird about using an alpha dependency. We can leave this as is for the sake of this pull request though.
|
I pushed some minor fixes here. I also made an additional pull request based against this one so you could review the change separately, it converts Koala to a class to match Koa 2's usage doug-wade#1. I'm having a hard time fixing the 404/500 errors in the test failures, can you take a look when you have a chance? We should also drop |
lib/index.js
Outdated
| this.use(etag()); | ||
| this.use(bodyParser()); | ||
| // this.use(convert(trace)); | ||
| const pug = new Pug({ |
There was a problem hiding this comment.
I will prefer passing a merged options.pug object to pug, something like new Pug(options.pug) with required defaults.
So that it can be configured by someone who is using this.
lib/middleware/conditional-get.js
Outdated
| var etag = require('koa-etag')(options); | ||
|
|
||
| return function* conditionalGet(next) { | ||
| return async function (next) { | ||
| yield* etag.call(this, next); |
There was a problem hiding this comment.
Why yield, why not await ?
test/app/body-parsing.js
Outdated
| @@ -2,7 +2,7 @@ | |||
| describe('Body Parsing', function () { | |||
| describe('.request.json()', function () { | |||
| it('should parse a json body', function (done) { | |||
| var app = koala() | |||
| var app = new Koala() | |||
| app.use(function* () { | |||
There was a problem hiding this comment.
Guess we can also move generators here to async/await ?
|
yo @nickmccurdy @bhaskarmelkani, I think this one's ready to merge. I'd appreciate a once-over if you've got a sec. |
|
Hey, Thanks for the updates. I completely missed the notification for your message. Looks good to me :) |
Fixes #17. Tested against http://github.com/doug-wade/koala-example