refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE] #125

dead-horse · 2017-11-06T15:45:35Z

Checklist

refactor router.js to adapt async function
refactor loader/mixin/controller.js and loader/mixin/middleware.js to adapt async function
drop support of node < 8
npm test passes
tests and/or benchmarks are included
commit message follows commit guidelines
pass all egg's test case
add more async test cases

Affected core subsystem(s)

Description of change

related eggjs/egg#1564

dead-horse · 2017-11-06T15:46:31Z

pass all the old test cases, need to check the code base one more time and add more async test cases.

fengmk2 · 2017-11-06T15:53:11Z

增加一个里程碑还是 project？ @popomore

codecov-io · 2017-11-06T15:54:24Z

Codecov Report

Merging #125 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #125   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         879    872    -7     
=====================================
- Hits          879    872    -7

Impacted Files	Coverage Δ
lib/utils/router.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/controller.js	`100% <100%> (ø)`	⬆️
lib/loader/mixin/middleware.js	`100% <100%> (ø)`	⬆️
lib/loader/file_loader.js	`100% <100%> (ø)`	⬆️
lib/utils/index.js	`100% <100%> (ø)`	⬆️
lib/egg.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21021f8...54abaef. Read the comment docs.

popomore · 2017-11-06T15:54:55Z

https:/orgs/eggjs/projects/2

fengmk2 · 2017-11-06T15:54:59Z

好吧，原来有的

dead-horse · 2017-11-07T02:04:51Z

benchmark/middleware/README.md

- Latency 14.39ms 1.61ms 30.73ms 93.22%
- Req/Sec 425.00 33.35 533.00 68.54%
- 33427 requests in 10.00s, 21.86MB read
-Requests/sec: 3342.23


对比基于 koa@1 的分支的测试结果，基本就是 async 和 generator 的结果对调

v8.9.0 server started at 7001 ------- generator middleware ------- ["generator middleware #1","generator middleware #2","generator middleware #3","generator middleware #4","generator middleware #5","generator middleware #6","generator middleware #7","generator middleware #8","generator middleware #9","generator middleware #10","generator middleware #11","generator middleware #12","generator middleware #13","generator middleware #14","generator middleware #15","generator middleware #16","generator middleware #17","generator middleware #18","generator middleware #19","generator middleware #20"] Running 10s test @ http://127.0.0.1:7001/generator 8 threads and 50 connections Thread Stats Avg Stdev Max +/- Stdev Latency 5.93ms 1.50ms 21.06ms 86.83% Req/Sec 1.05k 137.67 1.33k 78.41% 80170 requests in 9.99s, 59.26MB read Requests/sec: 8022.31 Transfer/sec: 5.93MB ------- async middleware ------- ["async middleware #1","async middleware #2","async middleware #3","async middleware #4","async middleware #5","async middleware #6","async middleware #7","async middleware #8","async middleware #9","async middleware #10","async middleware #11","async middleware #12","async middleware #13","async middleware #14","async middleware #15","async middleware #16","async middleware #17","async middleware #18","async middleware #19","async middleware #20"] Running 10s test @ http://127.0.0.1:7001/async 8 threads and 50 connections Thread Stats Avg Stdev Max +/- Stdev Latency 8.14ms 1.53ms 24.10ms 90.34% Req/Sec 756.97 71.92 0.96k 79.21% 58819 requests in 10.00s, 38.71MB read Requests/sec: 5881.22 Transfer/sec: 3.87MB

就看谁在 co wrap 中了，所以当前版本写 generator function 好，而下一个版本都建议改成 async function

恩，抓了个火焰图继续优化一下，现在应该 generator function 没去干净。

火焰图看起来已经很健康了，�性能大概要比上一个版本提升 10%+

dead-horse · 2017-11-07T03:32:30Z

link to egg and passed all egg's test cases.

dead-horse · 2017-11-07T05:25:35Z

@popomore @fengmk2 可以开始 review 了

fengmk2 · 2017-11-07T06:07:46Z

lib/egg.js

@@ -237,19 +235,19 @@ class EggCore extends KoaApplication {
 close() {
 if (this[CLOSE_PROMISE]) return this[CLOSE_PROMISE];


同一个 promise 可以被消费多次？

第二次就直接 resolve 了

看来是 promise 的规范约定？

这里改动应该和原来逻辑一致，会返回消费后的结果

atian25 · 2017-11-07T06:11:01Z

package.json

@@ -27,17 +27,17 @@
 },
 "homepage": "https:/eggjs/egg-core#readme",
 "engines": {
- "node": ">= 6.0.0"
+ "node": ">= 8.0.0"


这里不改 LTS 了，太麻烦，要做也只在 egg 做

fengmk2 · 2017-11-07T06:27:09Z

后面新的测试用例都需要改成 async 写。

popomore · 2017-11-07T06:30:38Z

lib/loader/file_loader.js

@@ -201,7 +201,7 @@ function getExports(fullpath, { initializer, call, inject }, pathName) {
 // module.exports = class Service {};
 // or
 // module.exports = function*() {}
- if (is.class(exports) || is.generatorFunction(exports)) {
+ if (is.class(exports) || is.generatorFunction(exports) || is.asyncFunction(exports)) {


这个还要支持？原来就不推荐这个写法。

这个如果不支持，通过 codemod 改掉的代码就不能跑了

exports 的方式要 break 掉? 兼容但不写到文档还是？

不过要 break 的话，以后写 codemod 就轻松多了。（有点诱惑）

策略一直都是兼容，但是不写到文档

dead-horse · 2017-11-07T06:30:47Z

后面新的测试用例都需要改成 async 写。

等这个 PR 合并了我再单独提一个修改，不然混到一起不好看。

popomore · 2017-11-07T06:39:47Z

lib/loader/mixin/middleware.js

- const fn = function* (next) {
- if (!match(this)) return yield next;
- yield mw.call(this, next);
+ const fn = (ctx, next) => {


这里加个 async 规范点？

koa 支持这种形式的，少一个 async 应该性能更好

应该一样的，async 如果返回一个 promise 也不会 wrap 的

popomore · 2017-11-07T06:41:04Z

lib/loader/mixin/middleware.js

- yield mw.call(this, next);
+ const fn = (ctx, next) => {
+ if (!match(ctx)) return next();
+ return utils.callFn(mw, [ ctx, next ]);


这里还需要用 callFn 吗，utils.middleware 应该已经返回了 promise

这里应该可以不用了，我试试

popomore · 2017-11-07T06:44:57Z

test/fixtures/async-controller-app/app/controller/async.js

@@ -1,16 +0,0 @@
-'use strict';
-
-var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {


保留一个编译后的用例吧

还有几个编译后的测试用例保留了，大部分都改成了 async function

popomore · 2017-11-07T06:49:56Z

lib/utils/router.js

@@ -298,7 +303,29 @@ function convertMiddlewares(middlewares, app) {
 });
 controller = obj;
 }
- return middlewares.concat([ controller ]);
+ let wrappedController;


这一段 controller 里面没做处理吗，应该已经 wrap 好了

popomore · 2017-11-07T06:53:59Z

callFn 直接用 toAsyncFunction？

popomore · 2017-11-07T07:01:24Z

等这个 PR 合并了我再单独提一个修改，不然混到一起不好看。

就留几个 generator 的兼容测试就好了

popomore · 2017-11-07T07:44:54Z

现在用 aa 的堆栈是不是更清晰了。

dead-horse · 2017-11-07T07:52:46Z

co 的堆栈没了，如果有 nextTick 的异步，还是会中断。

Error: error in async function
    at /Users/deadhorse/git/github.com/eggjs/egg-core/benchmark/middleware/app/middleware/async.js:9:28
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
Error: error in generator function
    at Object.<anonymous> (/Users/deadhorse/git/github.com/eggjs/egg-core/benchmark/middleware/app/middleware/generator.js:9:28)
    at Generator.next (<anonymous>)
    at onFulfilled (/Users/deadhorse/git/github.com/eggjs/egg-core/node_modules/[email protected]@co/index.js:65:19)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

dead-horse · 2017-11-07T08:06:53Z

就留几个 generator 的兼容测试就好了

现在的测试用例本来就是 generator 和 async 都测试了，所以基本不用怎么改 fixtures 了，等下提个 PR 把 mocha 中的 function* 改成 async function

dead-horse · 2017-11-07T08:13:05Z

callFn 直接用 toAsyncFunction？

里面还是直接用 co.wrap 吧，拿不到 app 对象

popomore · 2017-11-07T08:17:00Z

里面还是直接用 co.wrap 吧，拿不到 app 对象

功能一样吧，toAsyncFunction 直接用 callFn

dead-horse · 2017-11-07T08:20:00Z

还是不太一样的，toAsyncFunction 只做了 wrap， callFn wrap 之后还会执行，返回 Promise

popomore · 2017-11-07T08:22:30Z

相当于 toAsyncFunction(fn)(...args) ？

dead-horse · 2017-11-07T08:39:11Z

插件倒是有可能要做这个支持

dead-horse · 2017-11-07T08:40:18Z

toAsyncFunction 的优化单独一个 PR，我先另外提

popomore · 2017-11-07T08:40:50Z

而且插件最好是兼容更新，根据现在的 API 修改，然后升级到 v2 不需要修改。这种插件在 ci 中需要添加两个版本的 egg 测试，这个可以 egg-ci 添加一个参数，开启多版本测试。

dead-horse · 2017-11-07T08:53:46Z

@popomore #126

dead-horse · 2017-11-07T09:33:41Z

rebase 了 #126 的修改，再看看还有没有其他问题。

popomore · 2017-11-07T09:57:03Z

lib/utils/router.js

- controller = obj;
- }
- return middlewares.concat([ controller ]);
+ const wrappedController = async (ctx, next) => {


上面如果 module.exports = async () => {} 这个其实不是 koa2 中间件，只是 generator function 转成了 async function。

对于这两种形态应该做一些区分，如果是函数就是指 koa 中间件，那 koa1 和 koa2 应该有所区分，而不只是写法变化。这里在 controller 的 wrap 就应该区分这两个写法。

如果是 generator function，按照这里所写的进行 wrap，外层 async function 也是 koa 写法，generator function 是 koa1 写法。

如果是 async function，不用处理，只支持 koa2 的写法

其实我们已经将 controller 的写法重新定义过了，现在 controller 不论是 generator function 还是 async function，都是：

async function (ctx) { // this === ctx }

�在 controller 的 loader 有处理，只是 chair 内部支持的不一样，因为兼容行问题，没有支持 ctx 作为第一个参数。

那这里就不用 wrap 了？

需要 .bind(ctx)，也有可能是一个 generatorFunction（app.get(url, function*() {}))，不过这里可以去掉 async 和 await 改成 common function

dead-horse · 2017-11-07T15:54:40Z

lib/utils/router.js

- controller = obj;
- }
- return middlewares.concat([ controller ]);
+ const wrappedController = (ctx, next) => {


所有的 async () => await getPromise() 都可以变成 () => getPromise()，和 generator function 不同的点。

为啥原来不需要 bind？

为啥原来不需要 bind？

koa 1 中间件的形式 this 就是 ctx，koa 2 的 this 已经不是 ctx 了。由于我们的 controller 可能被其他插件复用，所以最好不修改 controller 的形式，在这里包一层，让 controller 保持现状。

Controller 会被插件复用？它不是只在应用中么

恩，我们会有其他的非 http 的入口场景复用了 controller。

喔，记得了

@dead-horse 这里有 bug 了，Controller 不存在的检测被延迟了。

popomore

其他 +1

dead-horse · 2017-11-08T01:43:34Z

@eggjs/core 有兴趣的同学再看看，没有问题我中午的时候合并 master 发 4.0.0，3.x 单独开一个分支维护。

popomore · 2017-11-08T02:22:34Z

还有一个 #127

dead-horse · 2017-11-08T03:30:45Z

合并发布了

dead-horse · 2017-11-08T03:32:36Z

4.0.0

)

dead-horse changed the title ~~refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE]~~ [WIP] refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE] Nov 6, 2017

popomore mentioned this pull request Nov 6, 2017

V2 eggjs/egg#1564

Closed

29 tasks

dead-horse commented Nov 7, 2017

View reviewed changes

dead-horse changed the title ~~[WIP] refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE]~~ refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE] Nov 7, 2017

fengmk2 reviewed Nov 7, 2017

View reviewed changes

atian25 reviewed Nov 7, 2017

View reviewed changes

fengmk2 approved these changes Nov 7, 2017

View reviewed changes

popomore reviewed Nov 7, 2017

View reviewed changes

dead-horse mentioned this pull request Nov 7, 2017

feat: toAsyncFunction compact with async function #126

Merged

4 tasks

dead-horse force-pushed the koa-2 branch from 8dc67df to accd409 Compare November 7, 2017 09:33

popomore reviewed Nov 7, 2017

View reviewed changes

dead-horse commented Nov 7, 2017

View reviewed changes

popomore reviewed Nov 8, 2017

View reviewed changes

popomore approved these changes Nov 8, 2017

View reviewed changes

dead-horse added 11 commits November 8, 2017 10:40

refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE]

bb74fff

bench: update benchmark

3073e20

bench: update benchmark

e1d5ada

bench: update benchmark

3345aa6

bench: update benchmark

b4c3109

refactor: more async

094436c

refactor: refactor router

53f93f8

f

2654e8e

f

42e3abe

f

adba67e

f

54abaef

dead-horse force-pushed the koa-2 branch from 5610911 to 54abaef Compare November 8, 2017 02:40

dead-horse merged commit ba0c9b9 into master Nov 8, 2017

dead-horse deleted the koa-2 branch November 8, 2017 03:31

hyj1991 pushed a commit to hyj1991/egg-core that referenced this pull request Sep 16, 2021

refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE] (eggjs#125

1afa9ee

)

		@@ -237,19 +235,19 @@ class EggCore extends KoaApplication {
		close() {
		if (this[CLOSE_PROMISE]) return this[CLOSE_PROMISE];

		@@ -1,16 +0,0 @@
		'use strict';

		var __awaiter = (this && this.__awaiter) \|\| function (thisArg, _arguments, P, generator) {

refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE] #125

refactor: upgrade to koa@2 and koa-router@7 [BREAKING_CHANGE] #125

Conversation

dead-horse commented Nov 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

dead-horse commented Nov 6, 2017

fengmk2 commented Nov 6, 2017

codecov-io commented Nov 6, 2017 • edited Loading

Codecov Report

popomore commented Nov 6, 2017

fengmk2 commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead-horse commented Nov 7, 2017

dead-horse commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengmk2 commented Nov 7, 2017

popomore Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atian25 Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dead-horse commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popomore commented Nov 7, 2017

popomore commented Nov 7, 2017

popomore commented Nov 7, 2017

dead-horse commented Nov 7, 2017

dead-horse commented Nov 7, 2017 • edited Loading

dead-horse commented Nov 7, 2017

popomore commented Nov 7, 2017

dead-horse commented Nov 7, 2017

popomore commented Nov 7, 2017

dead-horse commented Nov 7, 2017

dead-horse commented Nov 7, 2017

popomore commented Nov 7, 2017 • edited Loading

dead-horse commented Nov 7, 2017

dead-horse commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popomore left a comment

Choose a reason for hiding this comment

dead-horse commented Nov 8, 2017

popomore commented Nov 8, 2017

dead-horse commented Nov 8, 2017

dead-horse commented Nov 8, 2017

dead-horse commented Nov 6, 2017 •

edited

Loading

codecov-io commented Nov 6, 2017 •

edited

Loading

popomore Nov 7, 2017 •

edited

Loading

atian25 Nov 7, 2017 •

edited

Loading

dead-horse commented Nov 7, 2017 •

edited

Loading

popomore commented Nov 7, 2017 •

edited

Loading