Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug in Router#url & append Router object to ctx. #350

Merged
merged 3 commits into from
May 22, 2017

Conversation

HeavenDuke
Copy link
Contributor

I tried to fixed a bug found within Router#url under the latest version, where I get errors when trying to get a url with a name provided in an embedded router middleware, the change doesn't affect other parts(at least all test is passed :b ).
Also I find that in Koa2 you cannot access router object as previous version, where you can use:

router.get('book-detail', '/books/:book_id', function *() {
     // some code...
});
router.post('create-book', '/books', function *() {
    // some code...
    this.redirect(this.app.url('book-detail', {book_id: book.id}));
});

So I append the router object to ctx. This leads to the following usage:

router.get('book-detail', '/books/:book_id', function (ctx) {
    // some code...
});
router.post('create-book', '/books', function (ctx) {
    // some code...
    ctx.redirect(ctx.router.url('book-detail', {book_id: book.id}));
});

All test are passed under development environment.

@marvinhagemeister
Copy link

I'm curious as to why the test fail on node 4 + 5. I can't see anything not supported in older node versions with the changes in this PR.

@HeavenDuke
Copy link
Contributor Author

@marvinhagemeister maybe it's because I used async/await(since generator function is deprecated in koa2.x) but it seems that async/await is only supported in lower version of nodejs.

@marvinhagemeister
Copy link

@HeavenDuke I'm confused. There is no async/await included in the changeset or did I miss something obvious?

@HeavenDuke
Copy link
Contributor Author

It's just a guess, since the error log shows that the error arise from koa's lib/application.js, not from what I wrote.

@marvinhagemeister
Copy link

oh I see. That makes a lot of sense. Koa did drop support for older node versions with version 2 and requires a minimum of node v7.6.0. In this case I think we should drop travis testing for older node versions and align with the requirements of koa itself.

@HeavenDuke
Copy link
Contributor Author

right. I have dropped tests for 4 and 5, now everything works fine.

@jbielick jbielick merged commit 256fade into ZijianHe:master May 22, 2017
jbielick added a commit that referenced this pull request May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants