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

feat: app.close return a promise #19

Merged
merged 3 commits into from
Oct 19, 2016
Merged

feat: app.close return a promise #19

merged 3 commits into from
Oct 19, 2016

Conversation

popomore
Copy link
Member

@popomore popomore commented Oct 17, 2016

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

app.close

Description of change

@atian25
Copy link
Member

atian25 commented Oct 19, 2016

+1

this.emit('close');
this.removeAllListeners();
return new Promise((resolve, reject) => {
try {
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.

为什么要return promise

Copy link
Member Author

Choose a reason for hiding this comment

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

可能有异步的情况。

比如测试的时候可以 after(() => 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.

现在也可以这样写啊,close 是同步的

Copy link
Member

Choose a reason for hiding this comment

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

emit close 和 removeAllListeners 都是同步方法

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 Author

Choose a reason for hiding this comment

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

close() {
  return super
    .close()
    .then(resolve => {
      // close sth
      resolve();
    });
}

@codecov-io
Copy link

codecov-io commented Oct 19, 2016

Current coverage is 99.83% (diff: 100%)

Merging #19 into master will increase coverage by <.01%

@@             master        #19   diff @@
==========================================
  Files            16         16          
  Lines           612        616     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            611        615     +4   
  Misses            1          1          
  Partials          0          0          

Powered by Codecov. Last update f688b7f...d679851

@popomore
Copy link
Member Author

popomore commented Oct 19, 2016

Closes eggjs/egg#115

@fengmk2
Copy link
Member

fengmk2 commented Oct 19, 2016

+1

@fengmk2 fengmk2 merged commit 544adbc into master Oct 19, 2016
@fengmk2 fengmk2 deleted the close-promise branch October 19, 2016 16: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.

None yet

5 participants