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

refactor(midway-web): safelyGet() #269

Merged
merged 9 commits into from
Jul 9, 2019
Merged

Conversation

waitingsong
Copy link
Member

@waitingsong waitingsong commented Jul 8, 2019

  • add types
  • iterate with for-of instead of while
  • validate variable undefined with typeof
  • validate value undefined ahead of null check
  • return void 0 instead of undefined
Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

@codecov-io
Copy link

Codecov Report

Merging #269 into master will decrease coverage by 0.03%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
- Coverage    88.1%   88.07%   -0.04%     
==========================================
  Files          35       35              
  Lines         681      679       -2     
  Branches       47       47              
==========================================
- Hits          600      598       -2     
  Misses         71       71              
  Partials       10       10
Impacted Files Coverage Δ
packages/midway-web/src/utils.ts 87.87% <62.5%> (-0.7%) ⬇️

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 a9114d3...1a0d1cb. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jul 8, 2019

Codecov Report

Merging #269 into master will decrease coverage by 0.16%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
- Coverage    88.1%   87.94%   -0.17%     
==========================================
  Files          35       35              
  Lines         681      680       -1     
  Branches       47       47              
==========================================
- Hits          600      598       -2     
- Misses         71       72       +1     
  Partials       10       10
Impacted Files Coverage Δ
packages/midway-web/src/loader/webLoader.ts 90.9% <100%> (ø) ⬆️
packages/midway-web/src/utils.ts 85.29% <55.55%> (-3.28%) ⬇️

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 ddce869...9c48526. Read the comment docs.

@waitingsong waitingsong force-pushed the types branch 2 times, most recently from 748e991 to 8fbf356 Compare July 8, 2019 10:53
@waitingsong
Copy link
Member Author

waitingsong commented Jul 8, 2019

如此,写中间件时可这样:

export class ApiMiddleware implements WebMiddleware {

  public resolve(): KoaMiddleware {
    // ctx, next 不用单独写类型,通过上面一行的 KoaMiddleware 即可自动推导出类型
    return async (ctx, next) => {  
      // do something
      await next()
    }
  }

}

对比以前的方式,简洁多了:

export class ApiMiddleware implements WebMiddleware {

  public resolve() {
    return async (ctx: Context, next: () => Promise<void>) => {
      // do something
      await next()
    }
  }

}

@waitingsong
Copy link
Member Author

waitingsong commented Jul 8, 2019

基于底层的 egg 难道 ctx 不是 egg 的么。有点没明白。
另外,就是没找到 midway 的 context 等等类型所以才用 egg 的。

@czy88840616
Copy link
Member

随着迭代,现在你看到的 midway/ midway-web 可以理解为 midway for egg,在其他场景可能也会尝试,比如web 场景有 midway-koa/midway-express,其他场景也有比如midway-faas,midway-orm,其中很重要的一点是希望装饰器能尽可能重用,用户代码能够尽可能一样。

所以@midwayjs/decorator 可以看成是一个通用的装饰器定义,而实现是放在各个上次场景化框架中。

@waitingsong
Copy link
Member Author

明白。我之前也是准备用Koa的 Context。不过在上层的 egg 或者 midway 里面运行的时候就应该是他们(扩展出)的 context 了吧,那就需要在 egg 或者 midway 里面再扩展下类型吧?

@waitingsong
Copy link
Member Author

或者用泛型? 默认是 Koa 的 Context ,在 midway 里面传入个扩展的 Context .

@czy88840616
Copy link
Member

midway-web是有扩展出一个 context,不过装饰器这层就木有了

- add types
- iterate with for-of instead of while
- validate variable undefined with typeof
- validate value undefined ahead of null check
- return `void 0` instead of `undefined`
- safelyGet(['a','1'], {a: {"1": 2}})  // => 2
- safelyGet(['a','1'], {a: {b: 2}})  // => undefined
- update types of parameters of MidwayWebLoader.handlerWebMiddleware()
- fix `no-shadow` error relative to variable `middlewares`
@waitingsong
Copy link
Member Author

waitingsong commented Jul 9, 2019

这种如何:

import {Context} from 'koa';
export type KoaMiddleware <T extends any = Context> = (context: T, next: () => Promise<any>) => void;

不过目前无法传入泛型。比如 webLoader.ts 里面如果使用 KoaMiddleware<egg的Context> ,会导致类型错误(context上面缺少属性),并且 controller 这儿

 if (newRouter) {
      // implement middleware in controller
      const middlewares: MiddlewareParamArray = controllerOption.routerOptions.middleware;
如果改成
  const middlewares: MiddlewareParamArray<egg的Context> = controllerOption.routerOptions.middleware;

这儿的 middlewares 类型是 Koa的,如果不对应修改那么类型是不符的。

@czy88840616
Copy link
Member

看着好像没问题,不过这样用户端是不是就要改了。

@waitingsong
Copy link
Member Author

可能存在问题:
以前的 ctx 默认是 any 类型,用户要么直接用 any 要么自己指定一个(比如用 egg 的 Context)。
用这个泛型结构后如果用户在方法的ctx形参上面指定了 egg 那么可能就会和 KoaMiddleware 泛型默认的 koa 的 Context 不兼容吧。 等我测试下。
这个 context 泛型是不是应该整个统一入口呢……

@waitingsong
Copy link
Member Author

waitingsong commented Jul 9, 2019

假如 ctx 形参使用了 midway 导出的 Context 类型,那么就存在类型问题。
看来 midway 导出的是 egg 扩展后的 Context ,与 Koa 的类型不兼容。

2019-07-09_112530

@waitingsong
Copy link
Member Author

指定 ctx 类型而非 any 的目的是不用在形参那儿每次写一坨

 (ctx: Context, next: () => Promise<void>)

不过带来的问题是 decorator 就需要感知实际框架环境的 ctx 类型。

@czy88840616
Copy link
Member

decorator 固定几种类型,web 层面的ctx类型不多,应该就是 koaMiddleware/ExpressMiddleare

@waitingsong
Copy link
Member Author

如果默认用 koa 的 ctx,那么在 midway 框架中就无法获取到 egg 在 ctx 上扩展的属性类型了吧。不知道这个需求场景普遍不。

@czy88840616
Copy link
Member

czy88840616 commented Jul 9, 2019

这是我之前写的 express 的定义,看看能不能合进来😢

import * as express from 'express';

export interface WebMiddleware {
  resolve(): (req: express.Request, res: express.Response, next: express.NextFunction) => any;
}

export interface Application extends Partial<express.Application> {
  ready();
}

export type ExpressMiddleware = (req: express.Request, res: express.Response, next: express.NextFunction) => any;

亦或者涉及到这部分的,直接就放到各自的上层框架了。

@waitingsong
Copy link
Member Author

或者 decorator 那儿泛型用 any,midway 引入进来传个 egg 或者框架自己的 Context 类型,然后导出框架自身的 Middleware 类型供使用。

@czy88840616
Copy link
Member

czy88840616 commented Jul 9, 2019

对,这样比较好,具体类型由框架自己确定,用户用的时候,都是从框架导出去的。

@waitingsong
Copy link
Member Author

这样应该可行了。

@czy88840616 czy88840616 self-assigned this Jul 9, 2019
@czy88840616 czy88840616 merged commit a016e9b into midwayjs:master Jul 9, 2019
@waitingsong waitingsong deleted the types branch July 9, 2019 07:45
czy88840616 pushed a commit that referenced this pull request Jul 9, 2019
refactor(midway-web): safelyGet() (#269)

refactor(midway-web): safelyGet()
waitingsong added a commit to waitingsong/midway that referenced this pull request Jul 9, 2019
- split modal to config/config.modal.ts
- add ApiMiddleware demo used by HomeController
- ApiMiddleware use new exported type Middleware according to midwayjs#269
waitingsong added a commit to waitingsong/midway that referenced this pull request Jul 9, 2019
waitingsong added a commit to waitingsong/midway that referenced this pull request Jul 9, 2019
waitingsong added a commit to waitingsong/midway that referenced this pull request Jul 9, 2019
waitingsong added a commit to waitingsong/midway that referenced this pull request Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants