Skip to content

Commit

Permalink
Revert "feat: Added support for express@5 (newrelic#2555)"
Browse files Browse the repository at this point in the history
This reverts commit 15b7e5c.
  • Loading branch information
sumitsuthar authored Sep 20, 2024
1 parent a86162d commit 5330ace
Show file tree
Hide file tree
Showing 11 changed files with 72 additions and 57 deletions.
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 != null && (obj instanceof Object || (!obj.constructor && typeof obj === 'object'))
return obj instanceof Object
}

/**
Expand Down
24 changes: 10 additions & 14 deletions test/unit/shim/shim.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2545,20 +2545,16 @@ tap.test('Shim', function (t) {
t.beforeEach(beforeEach)
t.afterEach(afterEach)
t.test('should detect if an item is an object', function (t) {
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.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.end()
})
})
Expand Down
14 changes: 13 additions & 1 deletion test/versioned/express-esm/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,26 @@
},
"dependencies": {
"express": {
"versions": ">=4.6.0",
"versions": ">=4.6.0 && <5.0.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: 6 additions & 16 deletions test/versioned/express-esm/segments.tap.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ const assertSegmentsOptions = {
// const pkgVersion = expressPkg.version
import { readFileSync } from 'node:fs'
const { version: pkgVersion } = JSON.parse(readFileSync('./node_modules/express/package.json'))
const isExpress5 = semver.gte(pkgVersion, '5.0.0')
// TODO: change to 5.0.0 when officially released
const isExpress5 = semver.gte(pkgVersion, '5.0.0-beta.3')

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

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 = '/(.*)'
}

const path = isExpress5 ? '(.*)' : '*'
app.get(path, router1)

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

Expand Down
3 changes: 2 additions & 1 deletion test/versioned/express-esm/transaction-naming.tap.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ 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'))
const isExpress5 = semver.gte(pkgVersion, '5.0.0')
// TODO: change to 5.0.0 when officially released
const isExpress5 = semver.gte(pkgVersion, '5.0.0-beta.3')

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

/*
*
Expand All @@ -17,7 +16,7 @@ const { isExpress5 } = require('./utils')
*/
const COMPLETION = 27

test('Express async throw', { skip: isExpress5() }, function (t) {
test('Express async throw', 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, isExpress5 } = require('./utils')
const { makeRequest, setup } = require('./utils')
const { test } = require('tap')

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

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

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

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

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

const agent = helper.instrumentMockedAgent()
Expand Down
31 changes: 29 additions & 2 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",
"versions": ">=4.6.0 && <5.0.0",
"samples": 5
},
"express-enrouten": "1.1",
Expand All @@ -24,7 +24,6 @@
"files": [
"app-use.tap.js",
"async-error.tap.js",
"async-handlers.tap.js",
"bare-router.tap.js",
"captures-params.tap.js",
"client-disconnect.tap.js",
Expand All @@ -41,6 +40,34 @@
"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",
"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"
]
}
]
}
18 changes: 4 additions & 14 deletions test/versioned/express/segments.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,25 +243,15 @@ test('router mounted as a route handler', function (t) {
res.send('test')
})

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 = '/(.*)'
}
const path = isExpress5 ? '(.*)' : '*'
app.get(path, router1)

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

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

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

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

0 comments on commit 5330ace

Please sign in to comment.