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

转成 egg-core #6

Merged
merged 11 commits into from
Aug 15, 2016
Merged

转成 egg-core #6

merged 11 commits into from
Aug 15, 2016

Conversation

popomore
Copy link
Member

@popomore popomore commented Aug 9, 2016

转换成 egg-core,迁移 egg 的 contructor 部分,但是不处理与 loader 相关的(比如需要依赖 plugin 或 config)

eggjs/egg#19

包括

  • ready callback
  • console logger
  • logger
  • 监听 unhandledRejection

其他

  • 添加 loader.loadRouter
  • 去除判断 koa 的逻辑
  • egg-logger 的 consoleLogger 去掉打印到文件
  • 测试用例基于 EggCore 改造

This change is Reviewable

@mention-bot
Copy link

@popomore, thanks for your PR! By analyzing the annotation information on this pull request, we identified @gxcsoccer to be a potential reviewer

@fengmk2
Copy link
Member

fengmk2 commented Aug 9, 2016


lib/loader/mixin/extend.js, line 7 [r1] (raw file):

const interopRequire = require('interop-require');
const debug = require('debug')('egg-loader:extend');
const utils = require('../../utils');

这个 diff 好精细


Comments from Reviewable

@fengmk2
Copy link
Member

fengmk2 commented Aug 9, 2016

不过还是没 github 习惯,那个 review 太复杂了。

@popomore
Copy link
Member Author

popomore commented Aug 9, 2016

Review status: 0 of 17 files reviewed at latest revision, 2 unresolved discussions.


lib/loader/mixin/config.js, line 1 [r1] (raw file):

Previously, fengmk2 (fengmk2) wrote…

名称改成 config

啥名称,什么意思

Comments from Reviewable

@popomore
Copy link
Member Author

popomore commented Aug 9, 2016

应用 logger 不放 egg-core 了,因为也依赖 config

@popomore
Copy link
Member Author

没加 codecov�

@codecov-io
Copy link

codecov-io commented Aug 10, 2016

Current coverage is 99.67% (diff: 100%)

Merging #6 into master will decrease coverage by 0.12%

@@             master         #6   diff @@
==========================================
  Files            14         16     +2   
  Lines           498        612   +114   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            497        610   +113   
- Misses            1          2     +1   
  Partials          0          0          

Powered by Codecov. Last update 8b2debb...e2dee43

@popomore
Copy link
Member Author

codecov 终于来了

@@ -157,8 +156,9 @@ class Router extends KoaRouter {
if (opts.namePrefix) {
formatedName = opts.namePrefix + formatedName;
}

route.register.call(this, join(prefix, opts.suffix), [ opts.method ], middleware.concat(action), { name: formatedName });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

原来 Windows 下有 bug

@popomore
Copy link
Member Author

我把 reviewable 关掉吧,看来不太会用

@popomore
Copy link
Member Author

eggjs/egg-logger#6

@popomore popomore changed the title WIP: 转成 egg-core 转成 egg-core Aug 10, 2016
@popomore
Copy link
Member Author

可以 review 了


/**
* @constructor
* @param {Object} options - 创建应用配置
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[options]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[xx] 是默认值的意思吧

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可选的意思,默认值应该是[options={}]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

看了下是这样的,我改改

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

注释都一起改成英文?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options 这个 object 也是可选的, 加上 [options]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

创建应用配置 中文也改了吧

* ```
* @return {Route} return route object.
*/
resources(name, prefix, middleware) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个跟 egg-rest 有什么关系么

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一直有这个 api,我也想知道

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git blame 内网版看看是谁改的?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个比 egg-rest 早 @huacnlee

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是早期对 koa router 的扩展,跟 rest 还是不一样的,rest 是约定自动路由的,这个是要手动设置的。

@popomore
Copy link
Member Author

注释改了,再看看,我把 egg 依赖这个先改改

@dead-horse
Copy link
Member

以后 egg-core 就是 egg-loader 了?没跟上节奏。。

/**
* @constructor
* @param {Object} options - 创建应用配置
* @param {String} [options.baseDir=process.cwd()] - the directory of application
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里如果缩进的话应该不用 @param

- {String} [baseDir=process.cwd()]

不过这样也行

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我看例子推荐用 .

@atian25
Copy link
Member

atian25 commented Aug 12, 2016

app.coreLogger.info('Use coreMiddleware order: %j', this.config.coreMiddleware);
app.coreLogger.info('Use appMiddleware order: %j', this.config.appMiddleware);
this.options.logger.info('Use coreMiddleware order: %j', this.config.coreMiddleware);
this.options.logger.info('Use appMiddleware order: %j', this.config.appMiddleware);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么改成了 options.logger 而不是 app.coreLogger?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自己使用自己的,启动日志打到 stdout,这个时候有没有 coreLogger 还不一定

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get

@popomore
Copy link
Member Author

先 review 完我一起改

}

let app;
after(() => app && app.close());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

下面创建了2个 app,但是这里只 close 了第二个。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没看到哪里有两个

@fengmk2
Copy link
Member

fengmk2 commented Aug 12, 2016

ok了,基本没看出啥问题。 +1

@popomore
Copy link
Member Author

这里用例改的比较彻底了,终于改满意了

@popomore
Copy link
Member Author

改了,jsdoc 的细节等生成文档的时候再看吧

@fengmk2 fengmk2 merged commit 72e7ad7 into master Aug 15, 2016
@fengmk2 fengmk2 deleted the egg-core branch August 15, 2016 12:39
@fengmk2
Copy link
Member

fengmk2 commented Aug 15, 2016

@popomore good job!

@fengmk2
Copy link
Member

fengmk2 commented Aug 15, 2016

@popomore 改名 egg-core 发一个 0.1.0 ?

@popomore
Copy link
Member Author

我先改下仓库名

@popomore popomore restored the egg-core branch October 17, 2016 09:42
@popomore popomore deleted the egg-core branch December 19, 2016 09:37
fengmk2 added a commit that referenced this pull request Jan 13, 2023
```bash
egg start timeline:
▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [289ms] - #0 Process Start
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [510ms] - #1 Application Start
                  ▇ [2ms] - #2 Load Plugin
                  ▇ [1ms] - #3 Load Config
                  ▇ [0ms] - #4 Require(0) ~/eggjs/egg-core/test/fixtures/egg/config/config.default.js
                  ▇ [0ms] - #5 Require(1) ~/eggjs/egg-core/test/fixtures/egg/config/config.unittest.js
                  ▇ [0ms] - #6 Load extend/application.js
                  ▇ [0ms] - #7 Require(2) ~/eggjs/egg-core/test/fixtures/egg/app/extend/application.js
                  ▇ [1ms] - #8 Load extend/context.js
                  ▇ [0ms] - #9 Load extend/request.js
                  ▇ [0ms] - #10 Load extend/response.js
                  ▇ [1ms] - #11 Load app.js
                  ▇ [0ms] - #12 Require(3) ~/eggjs/egg-core/test/fixtures/egg/node_modules/session/app.js
                  ▇ [0ms] - #13 Require(4) app.js
                  ▇▇▇▇▇▇ [101ms] - #14 readyCallback in a
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [501ms] - #15 readyCallback in b
                  ▇ [4ms] - #16 Load Middleware
                  ▇ [4ms] - #17 Load "middlewares" to Application
                  ▇ [2ms] - #18 Load Service
                  ▇ [2ms] - #19 Load "service" to Context
                  ▇ [0ms] - #20 Load Controller
                  ▇ [0ms] - #21 Load "controller" to Application
                  ▇ [0ms] - #22 Load Router
```
fengmk2 added a commit that referenced this pull request Jan 13, 2023
```bash
egg start timeline:
▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [289ms] - #0 Process Start
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [510ms] - #1 Application Start
                  ▇ [2ms] - #2 Load Plugin
                  ▇ [1ms] - #3 Load Config
                  ▇ [0ms] - #4 Require(0) ~/eggjs/egg-core/test/fixtures/egg/config/config.default.js
                  ▇ [0ms] - #5 Require(1) ~/eggjs/egg-core/test/fixtures/egg/config/config.unittest.js
                  ▇ [0ms] - #6 Load extend/application.js
                  ▇ [0ms] - #7 Require(2) ~/eggjs/egg-core/test/fixtures/egg/app/extend/application.js
                  ▇ [1ms] - #8 Load extend/context.js
                  ▇ [0ms] - #9 Load extend/request.js
                  ▇ [0ms] - #10 Load extend/response.js
                  ▇ [1ms] - #11 Load app.js
                  ▇ [0ms] - #12 Require(3) ~/eggjs/egg-core/test/fixtures/egg/node_modules/session/app.js
                  ▇ [0ms] - #13 Require(4) app.js
                  ▇▇▇▇▇▇ [101ms] - #14 readyCallback in a
                  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ [501ms] - #15 readyCallback in b
                  ▇ [4ms] - #16 Load Middleware
                  ▇ [4ms] - #17 Load "middlewares" to Application
                  ▇ [2ms] - #18 Load Service
                  ▇ [2ms] - #19 Load "service" to Context
                  ▇ [0ms] - #20 Load Controller
                  ▇ [0ms] - #21 Load "controller" to Application
                  ▇ [0ms] - #22 Load Router
```
atian25 pushed a commit that referenced this pull request Jan 13, 2023
[skip ci]

## [5.3.0](v5.2.0...v5.3.0) (2023-01-13)

### Features

* support show app.timing in timline string ([#260](#260)) ([5b7af12](5b7af12)), closes [#0](https://github.com/eggjs/egg-core/issues/0) [#1](#1) [#2](#2) [#3](#3) [#4](#4) [#5](#5) [#6](#6) [#7](#7) [#8](#8) [#9](#9) [#10](#10) [#11](#11) [#12](#12) [#13](#13) [#14](#14) [#15](#15) [#16](#16) [#17](#17) [#18](#18) [#19](#19) [#20](#20) [#21](#21) [#22](#22)
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.

7 participants