emit more events #1395

tinovyatkin · 2019-10-13T17:03:04Z

Koa extends EventEmitter but doesn't use that fact for anything apart of error event.
However, there are lifecycle hooks that have real-world use and otherwise hard to implement. This PR adds 3 events: 'request', 'respond' and 'responded' as well as documenting the fact that Application is an EventEmitter in a more clear way.

With these events is also quite possible to build reactive requests handling on base of Koa.

codecov · 2019-10-13T17:06:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.79%. Comparing base (38cb591) to head (bc62b2a).
⚠️ Report is 183 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##            master    #1395      +/-   ##
===========================================
- Coverage   100.00%   99.79%   -0.21%     
===========================================
  Files            4        4              
  Lines          487      496       +9     
  Branches       136      137       +1     
===========================================
+ Hits           487      495       +8     
- Partials         0        1       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dead-horse · 2019-10-15T02:43:05Z

all these events can implemented in application.handleRequest:

handleRequest(ctx, fnMiddleware) {
    const res = ctx.res;
    res.statusCode = 404;
    const onerror = err => ctx.onerror(err);
    const handleResponse = () => {
      this.emit('respond', ctx);
      respond(ctx);
    }
    onFinished(res, err => {
      onerror(err);
      this.emit('responded', ctx);
    });
    this.emit('request', ctx);
    return fnMiddleware(ctx).then(handleResponse).catch(onerror);
  }

And I'm not sure if we really need to add the respond event which will make people confuse, I'd like to keep request and responded(then rename responded to respond):

handleRequest(ctx, fnMiddleware) {
    const res = ctx.res;
    res.statusCode = 404;
    const onerror = err => ctx.onerror(err);
    const handleResponse = () => respond(ctx);
    onFinished(res, err => {
      onerror(err);
      this.emit('respond', ctx);
    });
    this.emit('request', ctx);
    return fnMiddleware(ctx).then(handleResponse).catch(onerror);
  }

tinovyatkin · 2019-10-15T15:55:01Z

I would like to keep pre-send event ('respond' or if you suggest to rename it), as a good way to adjust context after all middleware but before sending to the network (there are some real world use for this event), don't see how it can be confusing while being documented, but if you have a rename suggestion - please do!

fl0w · 2019-10-15T22:23:35Z

Yea, sorry, I read this wrong.

lib/application.js

dead-horse · 2019-10-17T17:39:10Z

need test cases for this feature

docs/api/index.md

fengmk2 · 2019-10-18T01:09:46Z

lib/application.js

-    const onerror = err => ctx.onerror(err);
-    const handleResponse = () => respond(ctx);
+    const onerror = err => {
+      if (null != err) ctx.onerror(err);


This is breaking change.

What does it breaks?!

fengmk2 · 2019-10-19T07:45:12Z

lib/application.js

+    this.once(responding, () => this.emit('respond', ctx));
+
+    const onerror = err => {
+      if (null != err) {


the old code will execute onerror no matter the err is null or not. So I think this is a breaking change.

@fengmk2 onerror in current implementation just returns immediately if err is null, so, nothing breaks here actually.

jonathanong · 2020-04-28T04:19:04Z

docs/api/index.md

 - Many properties on `ctx` are defined using getters, setters, and `Object.defineProperty()`. You can only edit these properties (not recommended) by using `Object.defineProperty()` on `app.context`. See https://github.com/koajs/koa/issues/652.
 - Mounted apps currently use their parent's `ctx` and settings. Thus, mounted apps are really just groups of middleware.

+## Events


thanks for this doc, very useful for a PR

jonathanong · 2020-04-28T04:19:36Z

docs/api/index.md

+  })
+  ```
+
+### Event: 'respond'


i don't see how this or the request event have any benefit over regular middleware

it actually has less benefit because you cannot use async functions

There is no need to have advantage or disadvantage and not always sync call is inferior to async call from application point of view. If you follow Node.JS core development strategy then you certainly will notice “mix and match” innovation in handling async flow - once was added into events core module, finished, Readable.from(AsyncIterator), and Readable[AsyncIterator] into the stream - so, modern Node.JS is inviting to pick events, streams or Promises whatever solves problem more efficiently OR USE THEM ALTOGETHER. If Koa is the EventEmitter why it doesn’t emit any lifecycle events apart of error and why event handlers should be considered inferior?

jonathanong · 2020-04-28T04:20:18Z

docs/api/index.md

+  })
+  ```
+
+  Note: `respond` event may not be emitted in case of premature socket close (due to a middleware timeout, for example).


exceptions like this make it less useful and more difficult to maintain

It's not an exception - it's a logic of this event because the response in this case never happened.

jonathanong · 2020-04-28T04:21:17Z

docs/api/index.md

+
+  ```js
+  app.on('responded', ctx => {
+    if (ctx.state.dataStream) ctx.state.dataStream.destroy();


i don't understand this use-case. clean up callbacks on streams should be handled when you create and pipe them.

How would you know from the stream that, for example, connection with a client was prematurely lost and not the whole stream was consumed?

jonathanong · 2020-04-28T04:23:28Z

docs/api/index.md

+  More advanced example, use events to detect that server is idling for some time:
+
+  ```js
+    const server = app.listen();


i would actually recommend not doing this in koa. i would do this as a req/res decorator:

const fn = app.callback() const checkIdle = (fn) => (req, res) => { // do stuff return fn(req, res) } http.createServer(checkIdle(fn))

I probably missing something here as I’m struggling to understand: if you created Koa as an extension of EventEmitter in the first place, why do you consider using events is antipattern and inferior to writing low-level decorator here?

emit more events

83940d0

handle onerror res.end

4c4d064

tinovyatkin mentioned this pull request Oct 15, 2019

koa 3.0 roadmap #1114

Closed

move all events to handleRequest

82b5f1c

dead-horse reviewed Oct 17, 2019

View reviewed changes

lib/application.js Outdated Show resolved Hide resolved

tinovyatkin added 7 commits October 17, 2019 14:30

remove duplicated event emit

2faaa63

add test

7303f20

add custom event just in case

c719bdb

ensure once events and ctx param

0fc13d9

remove console.log

e4bf680

add error event test

2d181dd

Merge branch 'master' into emit-more-events

7190d4e

fengmk2 reviewed Oct 18, 2019

View reviewed changes

docs/api/index.md Show resolved Hide resolved

fengmk2 reviewed Oct 18, 2019

View reviewed changes

fengmk2 added the maybe-3.x label Oct 18, 2019

tinovyatkin added 4 commits October 17, 2019 21:27

clearInterval on responded in example

6b996ba

correct events in case of error

31911c4

use server.timeout in example

4107093

attach responded to the socket

4d70048

fengmk2 reviewed Oct 19, 2019

View reviewed changes

tinovyatkin added 2 commits October 28, 2019 09:44

Merge branch 'master' into emit-more-events

3b47e65

Merge branch 'master' into emit-more-events

1bdfebb

jonathanong reviewed Apr 28, 2020

View reviewed changes

tinovyatkin added 3 commits May 16, 2020 16:10

Merge branch 'master' into emit-more-events

0681713

Merge branch 'master' into emit-more-events

c3369bf

Merge branch 'master' into emit-more-events

bc62b2a

Uh oh!

emit more events #1395

Are you sure you want to change the base?

emit more events #1395

Conversation

tinovyatkin commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dead-horse commented Oct 15, 2019

Uh oh!

tinovyatkin commented Oct 15, 2019

Uh oh!

fl0w commented Oct 15, 2019

Uh oh!

Uh oh!

dead-horse commented Oct 17, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinovyatkin Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tinovyatkin commented Oct 13, 2019 •

edited

Loading

codecov bot commented Oct 13, 2019 •

edited

Loading

tinovyatkin Oct 19, 2019 •

edited

Loading