-
Notifications
You must be signed in to change notification settings - Fork 232
feat: add support for express v5 #4581
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, after the cloud-metadata-fetcher test change we discussed (to skip that using _is_express_incompat.js
.
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
I set myself a challenge to try to replace express in that test. The result is in c16773e Let me know what you think. It's okay for me to just skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav express
@trentm there are other tests that depend on express in order to provide a mock server. Some examples
apm-agent-nodejs/test/lambda/wrapper.test.js Line 165 in cd398aa
it is also used in lambda. I wouldn't hesitate to disable 1 or 2 but there are a few more. Also they affect other technologies and features like lambda. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some nits.
Co-authored-by: Trent Mick <[email protected]>
@@ -6,6 +6,12 @@ | |||
|
|||
'use strict'; | |||
|
|||
const isExpressIncompat = require('../_is_express_incompat')(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for reviewer: looking at https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html I thought it is okay to skip tests for lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
There are a couple of errors but seem unrelated:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav apollo-server-express
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
var semver = require('semver'); | ||
var once = require('once'); | ||
var pgVersion = require('pg/package.json').version; | ||
var pgVersion = safeGetPackageVersion('pg'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to reviewer: i think this is related to the fact [email protected]
added support for ESM imports. ref: https://github.com/brianc/node-postgres/blob/master/CHANGELOG.md#pg8150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is because its package.json uses "exports" now: https://github.com/brianc/node-postgres/blob/980752ce003b466a691d394e177c7d33a869e960/packages/pg/package.json#L22-L33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits. Mainly the docs/release-notes/index.md thing should get corrected.
Co-authored-by: Trent Mick <[email protected]>
… into 4238-express-v5
Checklist
docs/release-notes/index.md