feat: Added support for express@5 (#2555)

newrelic · Sep 10, 2024 · 252f3b2 · 252f3b2
1 parent 44edd0c
commit 252f3b2
Show file tree

Hide file tree

Showing 11 changed files with 57 additions and 72 deletions.
diff --git a/lib/shim/shim.js b/lib/shim/shim.js
@@ -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'))
 }
 
 /**

diff --git a/test/unit/shim/shim.test.js b/test/unit/shim/shim.test.js
@@ -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()
     })
   })

diff --git a/test/versioned/express-esm/package.json b/test/versioned/express-esm/package.json
@@ -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": {}

diff --git a/test/versioned/express-esm/segments.tap.mjs b/test/versioned/express-esm/segments.tap.mjs
@@ -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()
@@ -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']]
@@ -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`
     )
   })
 

diff --git a/test/versioned/express-esm/transaction-naming.tap.mjs b/test/versioned/express-esm/transaction-naming.tap.mjs
@@ -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()

diff --git a/test/versioned/express/async-error.tap.js b/test/versioned/express/async-error.tap.js
@@ -8,6 +8,7 @@
 const path = require('path')
 const test = require('tap').test
 const fork = require('child_process').fork
+const { isExpress5 } = require('./utils')
 
 /*
  *
@@ -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
 

diff --git a/test/versioned/express/async-handlers.tap.js b/test/versioned/express/async-handlers.tap.js
@@ -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
@@ -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
 

diff --git a/test/versioned/express/express-enrouten.tap.js b/test/versioned/express/express-enrouten.tap.js
@@ -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()

diff --git a/test/versioned/express/package.json b/test/versioned/express/package.json
@@ -15,7 +15,7 @@
       },
       "dependencies": {
         "express": {
-          "versions": ">=4.6.0 && <5.0.0",
+          "versions": ">=4.6.0",
           "samples": 5
         },
         "express-enrouten": "1.1",
@@ -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",

diff --git a/test/versioned/express/segments.tap.js b/test/versioned/express/segments.tap.js
@@ -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']]
@@ -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()

diff --git a/test/versioned/express/utils.js b/test/versioned/express/utils.js
@@ -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) {