Skip to content
This repository was archived by the owner on Mar 11, 2026. It is now read-only.

Introduce middleware directory#248

Merged
JustinBeckwith merged 5 commits intogoogleapis:masterfrom
ofrobots:make-middleware
Oct 28, 2018
Merged

Introduce middleware directory#248
JustinBeckwith merged 5 commits intogoogleapis:masterfrom
ofrobots:make-middleware

Conversation

@ofrobots
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots commented Oct 19, 2018

Only pay attention to the last commit: 8fb1ba8. This depends on #246, so those commits show up here too – ignore them.

We need to share code between logging-{winston,bunyan} to support express (and in the future other frameworks) middleware. This PR contains a preview of what that would look like. This code will not be used locally, but will be used by both logging-{winston,bunyan}.

@JustinBeckwith expressed that it he doesn't like the idea of a new module to share this code and would prefer if this went in here. I'm opening the issue now to get broader feedback from others before we go ahead too much on this path.

@stephenplusplus @DominicKramer @kinwa91 (and others intersted) PTAL. I'm interest what y'all think.

@ghost ghost assigned ofrobots Oct 19, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 19, 2018
@ofrobots ofrobots added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 19, 2018
@ofrobots
Copy link
Copy Markdown
Contributor Author

To spell out the pros/cons more clearly:

  • ➖users of logging get extra code and extra dependencies (@opencensus/propagation-stackdriver, on-finished)
  • ➕reduces the overhead of yet another module that needs to be maintained

Copy link
Copy Markdown

@soldair soldair left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread src/middleware/express/make-http-request.ts Outdated
Comment thread src/middleware/express/make-http-request.ts Outdated
Comment thread src/middleware/express/make-middleware.ts
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@c70629c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #248   +/-   ##
=========================================
  Coverage          ?   92.25%           
=========================================
  Files             ?       12           
  Lines             ?      633           
  Branches          ?       60           
=========================================
  Hits              ?      584           
  Misses            ?       35           
  Partials          ?       14
Impacted Files Coverage Δ
src/middleware/express/make-http-request.ts 100% <100%> (ø)
src/middleware/express/make-middleware.ts 100% <100%> (ø)
src/middleware/context.ts 100% <100%> (ø)

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 c70629c...4510b49. Read the comment docs.

@ghost ghost assigned jkwlui Oct 19, 2018
Comment thread src/http-request.ts Outdated
* limitations under the License.
*/

export interface HttpRequest {

This comment was marked as spam.

This comment was marked as spam.

@ofrobots ofrobots changed the title WIP: introduce middleware directory Introduce middleware directory Oct 26, 2018
Common code to be used by the logging-{winston,bunyan} libs.
@ofrobots ofrobots removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 26, 2018
@ofrobots
Copy link
Copy Markdown
Contributor Author

Added tests. This is ready for review. @JustinBeckwith @soldair PTAL.

@JustinBeckwith JustinBeckwith merged commit 2265097 into googleapis:master Oct 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants