Skip to content

fix: expose express middleware, add system-test#278

Merged
JustinBeckwith merged 6 commits into
googleapis:masterfrom
ofrobots:moar-middleware
Apr 17, 2019
Merged

fix: expose express middleware, add system-test#278
JustinBeckwith merged 6 commits into
googleapis:masterfrom
ofrobots:moar-middleware

Conversation

@ofrobots
Copy link
Copy Markdown
Contributor

The express middleware was previously not publicly exposed; do so.
Also add a system-test to validate the behaviour.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 12, 2019
@ofrobots ofrobots requested a review from DominicKramer March 12, 2019 00:24
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 12, 2019

Codecov Report

Merging #278 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   94.18%   94.31%   +0.13%     
==========================================
  Files           4        4              
  Lines          86       88       +2     
  Branches       15       15              
==========================================
+ Hits           81       83       +2     
  Misses          2        2              
  Partials        3        3
Impacted Files Coverage Δ
src/common.ts 97.95% <ø> (ø) ⬆️
src/index.ts 84.61% <100%> (+2.79%) ⬆️

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 3affbc3...373bb21. Read the comment docs.

@ofrobots
Copy link
Copy Markdown
Contributor Author

@JustinBeckwith the docs job failure seems like a linkinator bug.

@JustinBeckwith
Copy link
Copy Markdown
Contributor

Yeah, so taking a look - "bug" is a strong word :) This url, as of now, is a 404:
https://raw.githubusercontent.com/googleapis/nodejs-logging-winston/master/doc/images/request-bundling.png

Not sure what the right way to handle this is! We could:
a.) Ignore the error and hammer this in, knowing that it will exist after the PR lands
b.) Create a commit that adds the image first, then this one
c.) Try to do some bespoke thing specific to github

Any thoughts on how y'all think this should go?

@ofrobots
Copy link
Copy Markdown
Contributor Author

Hammer is least effort, let's go with that.

@JustinBeckwith
Copy link
Copy Markdown
Contributor

SGTM

@ofrobots ofrobots force-pushed the moar-middleware branch 2 times, most recently from b197702 to 7080dcf Compare March 14, 2019 19:21
@ofrobots
Copy link
Copy Markdown
Contributor Author

The post install check was finding a legitimate failure when the module is used with winston@2. I had to water down some of the declared types to make things work with both winston@2 and winston@3.

@ofrobots
Copy link
Copy Markdown
Contributor Author

Post install is continuing to find legitimate problems.

node_modules/@google-cloud/logging-winston/build/src/middleware/express.d.ts:27:21 - error TS2694: Namespace 'winston' has no exported member 'Logger'.

27     logger: winston.Logger;

I thought I had addressed this by removing the manual type definition for the return type for middleware function. It turns out that the compiler will generate an explicit type anyway.

I don't see a way to emit proper types and support both winston@2 and winston@3 with the same module.

The only option I see is to drop support for winston@2 in a semver major.

@ofrobots ofrobots added status: blocked Resolving the issue is dependent on other work. needs work This is a pull request that needs a little love. and removed status: blocked Resolving the issue is dependent on other work. labels Mar 15, 2019
@DominicKramer
Copy link
Copy Markdown
Contributor

@ofrobots Let me know whenever you want another review of this. Thanks.

ofrobots added a commit to ofrobots/nodejs-logging-winston that referenced this pull request Apr 10, 2019
BREAKING CHANGE: Drop support for winston2 as a semver major change.

Ref: googleapis#278
ofrobots added a commit to ofrobots/nodejs-logging-winston that referenced this pull request Apr 10, 2019
BREAKING CHANGE: Drop support for winston2 as a semver major change.

Ref: googleapis#278
ofrobots added a commit to ofrobots/nodejs-logging-winston that referenced this pull request Apr 12, 2019
BREAKING CHANGE: Drop support for winston2 as a semver major change.

Ref: googleapis#278
The express middleware was previously not publicly exposed; do so.
Also add a system-test to validate the behaviour.
@ofrobots
Copy link
Copy Markdown
Contributor Author

Had to drop require-inject and replace it with proxyquire because of this issue: https://gist.github.com/ofrobots/06270fe3f65ea93a4bda1a13348d8400

@ofrobots ofrobots requested a review from DominicKramer April 17, 2019 17:57
@ofrobots
Copy link
Copy Markdown
Contributor Author

@JustinBeckwith @bcoe I need an admin to land this. The docs failure is a known limitation of the docs test.

@JustinBeckwith JustinBeckwith merged commit dc17ad7 into googleapis:master Apr 17, 2019
@ofrobots ofrobots deleted the moar-middleware branch April 18, 2019 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. needs work This is a pull request that needs a little love.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants