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

feat: Added support for express@5 #2555

Merged
merged 1 commit into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/shim/shim.js
Original file line number Diff line number Diff line change
Expand Up @@ -1300,7 +1300,7 @@ function getName(obj) {
* @returns {boolean} True if the object is an Object, else false.
*/
function isObject(obj) {
return obj instanceof Object
return obj != null && (obj instanceof Object || (!obj.constructor && typeof obj === 'object'))
}

/**
Expand Down
24 changes: 14 additions & 10 deletions test/unit/shim/shim.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2545,16 +2545,20 @@ tap.test('Shim', function (t) {
t.beforeEach(beforeEach)
t.afterEach(afterEach)
t.test('should detect if an item is an object', function (t) {
t.ok(shim.isObject({}))
t.ok(shim.isObject([]))
t.ok(shim.isObject(arguments))
t.ok(shim.isObject(function () {}))
t.notOk(shim.isObject(true))
t.notOk(shim.isObject(false))
t.notOk(shim.isObject('foobar'))
t.notOk(shim.isObject(1234))
t.notOk(shim.isObject(null))
t.notOk(shim.isObject(undefined))
t.equal(shim.isObject({}), true)
t.equal(shim.isObject([]), true)
t.equal(shim.isObject(arguments), true)
t.equal(
shim.isObject(function () {}),
true
)
t.equal(shim.isObject(Object.create(null)), true)
t.equal(shim.isObject(true), false)
t.equal(shim.isObject(false), false)
t.equal(shim.isObject('foobar'), false)
t.equal(shim.isObject(1234), false)
t.equal(shim.isObject(null), false)
t.equal(shim.isObject(undefined), false)
t.end()
})
})
Expand Down
14 changes: 1 addition & 13 deletions test/versioned/express-esm/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,14 @@
},
"dependencies": {
"express": {
"versions": ">=4.6.0 && <5.0.0",
"versions": ">=4.6.0",
"samples": 5
}
},
"files": [
"segments.tap.mjs",
"transaction-naming.tap.mjs"
]
},
{
"engines": {
"node": ">=18"
},
"dependencies": {
"express": "5.0.0-beta.3"
},
"files": [
"segments.tap.mjs",
"transaction-naming.tap.mjs"
]
}
],
"dependencies": {}
Expand Down
22 changes: 16 additions & 6 deletions test/versioned/express-esm/segments.tap.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ const assertSegmentsOptions = {
// const pkgVersion = expressPkg.version
import { readFileSync } from 'node:fs'
const { version: pkgVersion } = JSON.parse(readFileSync('./node_modules/express/package.json'))
// TODO: change to 5.0.0 when officially released
const isExpress5 = semver.gte(pkgVersion, '5.0.0-beta.3')
const isExpress5 = semver.gte(pkgVersion, '5.0.0')

test('transaction segments tests', (t) => {
t.autoend()
Expand Down Expand Up @@ -215,14 +214,25 @@ test('transaction segments tests', (t) => {
res.send('test')
})

const path = isExpress5 ? '(.*)' : '*'
let path = '*'
let segmentPath = '/*'
let metricsPath = segmentPath

// express 5 router must be regular expressions
// need to handle the nuance of the segment vs metric name in express 5
if (isExpress5) {
path = /(.*)/
segmentPath = '/(.*)/'
metricsPath = '/(.*)'
}

app.get(path, router1)

const { rootSegment, transaction } = await runTest({ app, t })
t.assertSegments(
rootSegment,
[
`Expressjs/Route Path: /${path}`,
`Expressjs/Route Path: ${segmentPath}`,
[
'Expressjs/Router: /',
['Expressjs/Route Path: /test', [NAMES.EXPRESS.MIDDLEWARE + 'testHandler']]
Expand All @@ -234,8 +244,8 @@ test('transaction segments tests', (t) => {
checkMetrics(
t,
transaction.metrics,
[`${NAMES.EXPRESS.MIDDLEWARE}testHandler//${path}/test`],
`/${path}/test`
[`${NAMES.EXPRESS.MIDDLEWARE}testHandler/${metricsPath}/test`],
`${metricsPath}/test`
)
})

Expand Down
3 changes: 1 addition & 2 deletions test/versioned/express-esm/transaction-naming.tap.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ const { setup, makeRequest, makeRequestAndFinishTransaction } = expressHelpers
// const pkgVersion = expressPkg.version
import { readFileSync } from 'node:fs'
const { version: pkgVersion } = JSON.parse(readFileSync('./node_modules/express/package.json'))
// TODO: change to 5.0.0 when officially released
const isExpress5 = semver.gte(pkgVersion, '5.0.0-beta.3')
const isExpress5 = semver.gte(pkgVersion, '5.0.0')

test('transaction naming tests', (t) => {
t.autoend()
Expand Down
3 changes: 2 additions & 1 deletion test/versioned/express/async-error.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
const path = require('path')
const test = require('tap').test
const fork = require('child_process').fork
const { isExpress5 } = require('./utils')

/*
*
Expand All @@ -16,7 +17,7 @@ const fork = require('child_process').fork
*/
const COMPLETION = 27

test('Express async throw', function (t) {
test('Express async throw', { skip: isExpress5() }, function (t) {
const erk = fork(path.join(__dirname, 'erk.js'))
let timer

Expand Down
6 changes: 3 additions & 3 deletions test/versioned/express/async-handlers.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

'use strict'

const { makeRequest, setup } = require('./utils')
const { makeRequest, setup, isExpress5 } = require('./utils')
const { test } = require('tap')

test('should properly track async handlers', (t) => {
test('should properly track async handlers', { skip: !isExpress5() }, (t) => {
setup(t)
const { app } = t.context
const mwTimeout = 20
Expand Down Expand Up @@ -44,7 +44,7 @@ test('should properly track async handlers', (t) => {
})
})

test('should properly handle errors in async handlers', (t) => {
test('should properly handle errors in async handlers', { skip: !isExpress5() }, (t) => {
setup(t)
const { app } = t.context

Expand Down
3 changes: 2 additions & 1 deletion test/versioned/express/express-enrouten.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@

const test = require('tap').test
const helper = require('../../lib/agent_helper')
const { isExpress5 } = require('./utils')

test('Express + express-enrouten compatibility test', function (t) {
test('Express + express-enrouten compatibility test', { skip: isExpress5() }, function (t) {
t.plan(2)

const agent = helper.instrumentMockedAgent()
Expand Down
31 changes: 2 additions & 29 deletions test/versioned/express/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
},
"dependencies": {
"express": {
"versions": ">=4.6.0 && <5.0.0",
"versions": ">=4.6.0",
"samples": 5
},
"express-enrouten": "1.1",
Expand All @@ -24,39 +24,12 @@
"files": [
"app-use.tap.js",
"async-error.tap.js",
"bare-router.tap.js",
"captures-params.tap.js",
"client-disconnect.tap.js",
"errors.tap.js",
"express-enrouten.tap.js",
"ignoring.tap.js",
"issue171.tap.js",
"middleware-name.tap.js",
"render.tap.js",
"require.tap.js",
"route-iteration.tap.js",
"route-param.tap.js",
"router-params.tap.js",
"segments.tap.js",
"transaction-naming.tap.js"
]
},
{
"engines": {
"node": ">=18"
},
"dependencies": {
"express": "5.0.0-beta.3",
"ejs": "2.5.9"
},
"files": [
"app-use.tap.js",
"async-handlers.tap.js",
"async-error.tap.js",
"bare-router.tap.js",
"captures-params.tap.js",
"client-disconnect.tap.js",
"errors.tap.js",
"express-enrouten.tap.js",
"ignoring.tap.js",
"issue171.tap.js",
"middleware-name.tap.js",
Expand Down
18 changes: 14 additions & 4 deletions test/versioned/express/segments.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,15 +243,25 @@ test('router mounted as a route handler', function (t) {
res.send('test')
})

const path = isExpress5 ? '(.*)' : '*'
let path = '*'
let segmentPath = '/*'
let metricsPath = segmentPath

// express 5 router must be regular expressions
// need to handle the nuance of the segment vs metric name in express 5
if (isExpress5) {
path = /(.*)/
segmentPath = '/(.*)/'
metricsPath = '/(.*)'
}
app.get(path, router1)

runTest(t, '/test', function (segments, transaction) {
checkSegments(
t,
transaction.trace.root.children[0],
[
`Expressjs/Route Path: /${path}`,
`Expressjs/Route Path: ${segmentPath}`,
[
'Expressjs/Router: /',
['Expressjs/Route Path: /test', [NAMES.EXPRESS.MIDDLEWARE + 'testHandler']]
Expand All @@ -263,8 +273,8 @@ test('router mounted as a route handler', function (t) {
checkMetrics(
t,
transaction.metrics,
[`${NAMES.EXPRESS.MIDDLEWARE}testHandler//${path}/test`],
`/${path}/test`
[`${NAMES.EXPRESS.MIDDLEWARE}testHandler/${metricsPath}/test`],
`${metricsPath}/test`
)

t.end()
Expand Down
3 changes: 1 addition & 2 deletions test/versioned/express/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ const semver = require('semver')

function isExpress5() {
const { version } = require('express/package')
// TODO: change to 5.0.0 when officially released
return semver.gte(version, '5.0.0-beta.3')
return semver.gte(version, '5.0.0')
}

function makeRequest(server, path, callback) {
Expand Down
Loading