Skip to content

better demonstrate middleware flow#1195

Merged
dead-horse merged 1 commit intokoajs:masterfrom
jasonmacgowan:jasonmacgowan-patch-1
Jun 6, 2018
Merged

better demonstrate middleware flow#1195
dead-horse merged 1 commit intokoajs:masterfrom
jasonmacgowan:jasonmacgowan-patch-1

Conversation

@jasonmacgowan
Copy link
Copy Markdown
Contributor

The way this example is written makes it seem like middleware can't act on the context changes of other middleware. This updates the logger middleware to use the calculated response time value from the response time middleware instead of calculating it itself.

It's not incredibly clear from this example that middleware can act on context changes from other middleware.  This updates the logger middleware to use the calculated response time value from the response time middleware instead of calculating it twice.
@codecov
Copy link
Copy Markdown

codecov bot commented May 24, 2018

Codecov Report

Merging #1195 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1195   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files           5        5           
  Lines         374      374           
=======================================
  Hits          373      373           
  Misses          1        1

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 148f26f...9e48613. Read the comment docs.

@fl0w
Copy link
Copy Markdown
Contributor

fl0w commented May 30, 2018

I'm +/-0 on this.

@doug-wade
Copy link
Copy Markdown
Contributor

I get where you're headed on this, and I dig it, but wouldn't this make the X-Response-Time ever so slightly too short?

@jasonmacgowan
Copy link
Copy Markdown
Contributor Author

@doug-wade measured in milliseconds in this simple of an example I'd be surprised if there was a difference. If there was a difference, we are already logging one value and sending back another so I'd imagine it's a non-issue.

Comment thread docs/api/index.md
const ms = Date.now() - start;
ctx.set('X-Response-Time', `${ms}ms`);
const rt = ctx.response.get('X-Response-Time');
console.log(`${ctx.method} ${ctx.url} - ${rt}`);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's not good here because rt is a string

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dead-horse I'm not sure what you mean; can you elaborate please? Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

never mind, my misunderstanding.

@dead-horse dead-horse merged commit 4d42500 into koajs:master Jun 6, 2018
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.

4 participants