feat: add biz handler #29
Conversation
There is three case: 1. If it's an egg error, it will format response 1. If it's an egg exception, it will format response and log an error 1. if it's a normal error, it will be throw that catched by onerror handler
|
先看看处理是否合适。 |
|
非 json 的没想好怎么处理，内容协商 egg 做的不是很好。 |
|
@popomore 非 json 的本来 koa-onerror 就已经处理的了，得复用起来。 |
|
这是是在前面拦截统一处理的，onerror 就是未捕获异常的处理，所以我觉得业务异常都可以在这里提供处理的扩张。 |
|
是不是提供这样的配置，让开发者明确是处理哪种类型，格式化统一只给一个 error 对象。 onerror.uncaught.formatJSON |
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
==========================================
+ Coverage 96.98% 97.53% +0.54%
==========================================
Files 5 6 +1
Lines 166 203 +37
==========================================
+ Hits 161 198 +37
Misses 5 5
Continue to review full report at Codecov.
|
|
代码又改了下 |
| if (contentType === 'html' && biz.formatHtml) { | ||
| ctx.body = biz.formatHtml(err); | ||
| } else { | ||
| ctx.body = defaultFormat(err); |
There was a problem hiding this comment.
defaultFormat 直接交给 koa-onerror？
|
看看系统的是否也要加，因为这个是兜底的，如果出现了异常就会真挂掉。 还有一个纠结点是业务处理现在是无法处理默认的 Error 的，如果不提供系统的 format，那这个 error 处理只能使用 onerror 默认的了。 |
|
应用也可以将默认的 Error 处理了。按道理应用只处理自己的异常。 |
|
@popomore 清晰多了，先将文档写出来，文档里面将3种场景都写出示例代码，从开发者角度看看整个过程是否合理。 |
| @@ -11,4 +11,10 @@ exports.onerror = { | |||
| appErrorFilter: null, | |||
There was a problem hiding this comment.
上面的 // 5xx error will redirect to ${errorPageUrl} 注释都需要修改一下了。
config/config.default.js
Outdated
| @@ -11,4 +11,10 @@ exports.onerror = { | |||
| appErrorFilter: null, | |||
| // default template path | |||
| templatePath: path.join(__dirname, '../lib/onerror_page.mustache'), | |||
| // normalize your error response from error object | |||
There was a problem hiding this comment.
ErrorType.ERROR and ErrorType.EXCEPTION
|
这个放在 onerror 插件里面给我的感觉还是有点奇怪，因为里面内嵌了一些 egg-errors 的逻辑，需要配套使用。我觉得还不如单独实现一个 egg-biz-error 的插件，统一提供 egg-errors 和对应的中间件处理，给用户的感知是统一的。 |
|
例如在这里配置 application.formatXX 方法， 但是一些 builtin 的异常又没有按照这个格式来处理，会让人有点疑惑。 如果是新增一个 egg-biz-error 插件，所有通过插件提供的方式抛出的异常都会被这个插件处理，系统 builtin 异常不会处理，就比较合理。 egg-biz-error 插件如果接口稳定的话，也可以在 egg 中默认开启 |
|
我先把文档写一下，这样比较容易理解。 |
|
egg-errors 单独使用是为了让库、插件、框架、应用都可以单独依赖来使用，而不是依赖一个 egg-biz-error 的插件。 抽出 biz-error 插件做这个错误处理我觉得也没有问题，但是这个属于 egg 错误处理的核心功能，所以我觉得再抽个插件比较麻烦。 |
这个我是这样想的，builtin 异常可能出现在一些依赖库里，业务需要对其做处理，如果不做处理就认为是未捕获异常。 try {
// 调用依赖库
} catch (err) {
throw MyException.from(err);
} |
|
link 下 RFC 或文档，从使用者角度来看下，可能会更容易理解下。 |
|
我理解 egg-errors 作为单独库的的目的，我的意思是如果确定下来，可以直接将 egg-errors 从 egg-core 开始内置，这样在 egg-onerror 插件里面处理 egg-errors 相关的东西就合适了。 |
|
不是内置，是希望所有的错误都用这种方式封装。 |
| @@ -11,4 +11,24 @@ exports.onerror = { | |||
| appErrorFilter: null, | |||
| // default template path | |||
| templatePath: path.join(__dirname, '../lib/onerror_page.mustache'), | |||
| // handler your error response | |||
| errorHandler: { | |||
There was a problem hiding this comment.
感觉这个应该是一个配置，而是约定在 app/onerror.js 文件入口。
There was a problem hiding this comment.
我感觉这里比较简单，写配置比较容易想到，约定文件太多也不是非常好。配置还有一个好处是有 dump。
|
我们是不是需要提供默认项，如果开启了 errorHandler 而使用默认值的时候会跟原来 onerror 的默认处理有差异。 |
| let err = e; | ||
| const errorType = EggError.getType(err); | ||
| switch (errorType) { | ||
| case ErrorType.ERROR: break; |
There was a problem hiding this comment.
又看了一次代码，还是很难理解，日志什么时候该打，什么时候该使用 EggError，什么时候用 EXCEPTION 。。。
There was a problem hiding this comment.
Error 是用于终端的，比如检验错误，验权，不需要打 error。exception 是系统错误，常见的 500，或者底层抛的错，需要打 error 日志。针对响应则用 errorHandler 统一处理。
There was a problem hiding this comment.
先整理个文章出来吧，这种经验实践类的功能看代码老是跟不上思路。。。
There was a problem hiding this comment.
我理解的是 ERROR 是类似业务处理报错，是业务上预期的错误，EXCEPTION 是异常，非预期的程序错误，所以需要打印日志。
There was a problem hiding this comment.
如果是终端的，那么应该使用 ClientError 会更好，不用思考就明白了。
| "stack-trace": "^0.0.10" | ||
| }, | ||
| "devDependencies": { | ||
| "autod": "^3.0.0", | ||
| "autod": "^3.0.1", | ||
| "egg": "next", |
|
|
||
| class HomeController extends Controller { | ||
| async throwError() { | ||
| throw new Error('error'); |
There was a problem hiding this comment.
ctx.throw([status], [msg], [properties]) 和 ctx.assert(value, [status], [msg], [properties]) 这 2 个对应的用法也可以加下
There was a problem hiding this comment.
这个还是不要改原生了？就作为 BUILDIN 错误好了
| [ 'all', 'html', 'json', 'text', 'js' ].forEach(type => { | ||
| if (config[type]) errorOptions[type] = config[type]; | ||
| }); | ||
| onerror(app, errorOptions); |
There was a problem hiding this comment.
这里是不是可以再包一层，这样的话，在 custom error handler 里面，可以支持 ctx.render 的兜底处理
There is three case:
Checklist
npm testpassesAffected core subsystem(s)
Description of change