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

better demonstrate middleware flow #1195

Merged
merged 1 commit into from
Jun 6, 2018
Merged

better demonstrate middleware flow #1195

merged 1 commit into from
Jun 6, 2018

Conversation

jasonmacgowan
Copy link
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

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
Contributor

fl0w commented May 30, 2018

I'm +/-0 on this.

@doug-wade
Copy link
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
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.

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
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
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
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.

None yet

4 participants