-
Notifications
You must be signed in to change notification settings - Fork 93
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(router): support app.get(url, controllerName) #38
Conversation
@dead-horse, thanks for your PR! By analyzing the history of the files in this pull request, we identified @popomore and @whxaxes to be potential reviewers. |
Current coverage is 99.40% (diff: 100%)
|
@@ -1,6 +1,6 @@ | |||
module.exports = function (app) { | |||
app.get('/locals/router', app.controller.locals.router); | |||
app.get('member_index', '/members/index', 'members.index'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不再支持 name, path, action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
支持啊,那个例子和下面重复了
if (typeof controller !== 'string') return args; | ||
const actions = controller.split('.'); | ||
let obj = app.controller; | ||
actions.forEach(function(key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arrow function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
/** | ||
* controller(last argument) support string | ||
* - [url, controller]: app.get('/home', 'home'); | ||
* - [name, url, controller]: app.get('home', '/home', 'home'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
最后那个用 news.list 区别下?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
news.list 是啥?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.get('home', '/home', 'home');
前后各一个 home, 要不要换个名字, 如
app.get('newsList', '/news/list', 'news.list');
这样的? 不过这样也行了.
4a15940
to
4bd5145
Compare
4bd5145
to
4aa93fd
Compare
const actions = controller.split('.'); | ||
let obj = app.controller; | ||
actions.forEach(key => { | ||
obj = obj[key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果指定一个不存在的 controller 呢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
加了一个 throw ,提示友好一点
actions.forEach(key => { | ||
obj = obj[key]; | ||
}); | ||
return Array.prototype.slice.call(args, 0, -1).concat([ obj ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
直接用 args[args.length - 1] = obj 不行吗
@@ -129,14 +138,10 @@ class Router extends KoaRouter { | |||
const controller = middleware.pop(); | |||
|
|||
for (const key in REST_MAP) { | |||
let action = ''; | |||
if (typeof controller === 'string') { | |||
action = `${controller}.${key}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个是不兼容?记录下
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
兼容的,这段逻辑等于是统一到下面那个方法里面了,进到这里的时候已经都解析成对象了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Checklist
npm test
passesAffected core subsystem(s)
Description of change
closes eggjs/egg#201