Skip to content

Commit 73c9946

Browse files
logaretmclaude
andcommitted
fix: use shouldTrace guard for Node 18 compatibility
Node 18 backported TracingChannel but without the aggregated `hasSubscribers` getter (it returns `undefined` instead of a boolean). Raw truthiness checks treat `undefined` as "no subscribers" which silently disables tracing on Node 18. Replace all `channel.hasSubscribers` guards with `shouldTrace(channel)` which checks `hasSubscribers !== false` — treating `undefined` (Node 18) as "might have subscribers, trace unconditionally" and `false` (Node 20+) as "definitely no subscribers, skip". Also removes the now-unnecessary test skip logic since TracingChannel does exist on Node 18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 5b50561 commit 73c9946

6 files changed

Lines changed: 35 additions & 25 deletions

File tree

packages/pg-pool/diagnostics.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,12 @@ try {
2424
// diagnostics_channel not available (non-Node environment)
2525
}
2626

27-
module.exports = { poolConnectChannel, poolReleaseChannel, poolRemoveChannel }
27+
// Check explicitly for `false` rather than truthiness because the aggregated
28+
// `hasSubscribers` getter on TracingChannel is `undefined` on Node 18 (which
29+
// backported TracingChannel but not the getter). When `undefined`, we assume
30+
// there may be subscribers and trace unconditionally.
31+
function shouldTrace(channel) {
32+
return channel.hasSubscribers !== false
33+
}
34+
35+
module.exports = { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace }

packages/pg-pool/index.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use strict'
22
const EventEmitter = require('events').EventEmitter
3-
const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel } = require('./diagnostics')
3+
const { poolConnectChannel, poolReleaseChannel, poolRemoveChannel, shouldTrace } = require('./diagnostics')
44

55
const NOOP = function () {}
66

@@ -179,7 +179,7 @@ class Pool extends EventEmitter {
179179

180180
this._clients = this._clients.filter((c) => c !== client)
181181
const context = this
182-
if (poolRemoveChannel.hasSubscribers) {
182+
if (shouldTrace(poolRemoveChannel)) {
183183
poolRemoveChannel.publish({ client: { processID: client.processID } })
184184
}
185185

@@ -201,7 +201,7 @@ class Pool extends EventEmitter {
201201
const response = promisify(this.Promise, cb)
202202
const result = response.result
203203

204-
if (poolConnectChannel.hasSubscribers) {
204+
if (shouldTrace(poolConnectChannel)) {
205205
const context = {
206206
pool: {
207207
totalCount: this.totalCount,
@@ -418,7 +418,7 @@ class Pool extends EventEmitter {
418418

419419
this.emit('release', err, client)
420420

421-
if (poolReleaseChannel.hasSubscribers) {
421+
if (shouldTrace(poolReleaseChannel)) {
422422
poolReleaseChannel.publish({ client: { processID: client.processID }, error: err || undefined })
423423
}
424424

packages/pg-pool/test/diagnostics.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ const it = require('mocha').it
77
const dc = require('diagnostics_channel')
88
const Pool = require('../')
99

10-
const hasTracingChannel = typeof dc.tracingChannel === 'function'
11-
1210
function mockClient(methods) {
1311
return function () {
1412
const client = new EventEmitter()
@@ -25,7 +23,7 @@ function mockClient(methods) {
2523

2624
describe('diagnostics channels', function () {
2725
describe('pg:pool:connect', function () {
28-
;(hasTracingChannel ? it : it.skip)('publishes start event when connect is called', function (done) {
26+
it('publishes start event when connect is called', function (done) {
2927
const pool = new Pool({
3028
Client: mockClient({
3129
connect: function (cb) {
@@ -62,7 +60,7 @@ describe('diagnostics channels', function () {
6260
})
6361
})
6462
})
65-
;(hasTracingChannel ? it : it.skip)('enriches context with client info on asyncEnd', function (done) {
63+
it('enriches context with client info on asyncEnd', function (done) {
6664
const pool = new Pool({
6765
Client: mockClient({
6866
connect: function (cb) {

packages/pg/lib/client.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const Query = require('./query')
99
const defaults = require('./defaults')
1010
const Connection = require('./connection')
1111
const crypto = require('./crypto/utils')
12-
const { queryChannel, connectionChannel } = require('./diagnostics')
12+
const { queryChannel, connectionChannel, shouldTrace } = require('./diagnostics')
1313

1414
const activeQueryDeprecationNotice = nodeUtils.deprecate(
1515
() => {},
@@ -208,7 +208,7 @@ class Client extends EventEmitter {
208208

209209
connect(callback) {
210210
if (callback) {
211-
if (connectionChannel.hasSubscribers) {
211+
if (shouldTrace(connectionChannel)) {
212212
const context = {
213213
connection: { database: this.database, host: this.host, port: this.port, user: this.user, ssl: !!this.ssl },
214214
}
@@ -227,7 +227,7 @@ class Client extends EventEmitter {
227227
})
228228
})
229229

230-
if (connectionChannel.hasSubscribers) {
230+
if (shouldTrace(connectionChannel)) {
231231
const context = {
232232
connection: { database: this.database, host: this.host, port: this.port, user: this.user, ssl: !!this.ssl },
233233
}
@@ -708,7 +708,7 @@ class Client extends EventEmitter {
708708
this._pulseQueryQueue()
709709
}
710710

711-
if (queryChannel.hasSubscribers) {
711+
if (shouldTrace(queryChannel)) {
712712
const context = {
713713
query: { text: query.text, name: query.name, rowMode: query._rowMode },
714714
client: {

packages/pg/lib/diagnostics.js

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,12 @@ try {
2020
// diagnostics_channel not available (non-Node environment)
2121
}
2222

23-
module.exports = { queryChannel, connectionChannel }
23+
// Check explicitly for `false` rather than truthiness because the aggregated
24+
// `hasSubscribers` getter on TracingChannel is `undefined` on Node 18 (which
25+
// backported TracingChannel but not the getter). When `undefined`, we assume
26+
// there may be subscribers and trace unconditionally.
27+
function shouldTrace(channel) {
28+
return channel.hasSubscribers !== false
29+
}
30+
31+
module.exports = { queryChannel, connectionChannel, shouldTrace }

packages/pg/test/unit/client/diagnostics-tests.js

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,11 @@ const helper = require('./test-helper')
33
const assert = require('assert')
44
const dc = require('diagnostics_channel')
55

6-
const hasTracingChannel = typeof dc.tracingChannel === 'function'
7-
86
const suite = new helper.Suite()
97
const test = suite.test.bind(suite)
10-
// pass undefined as callback to skip when TracingChannel is unavailable
11-
const testTracing = (name, cb) => test(name, hasTracingChannel ? cb : undefined)
128

13-
testTracing('query diagnostics channel', function () {
14-
testTracing('publishes start and asyncEnd on successful query', function (done) {
9+
test('query diagnostics channel', function () {
10+
test('publishes start and asyncEnd on successful query', function (done) {
1511
const client = helper.client()
1612
client.connection.emit('readyForQuery')
1713

@@ -54,7 +50,7 @@ testTracing('query diagnostics channel', function () {
5450
client.connection.emit('readyForQuery')
5551
})
5652

57-
testTracing('publishes error on failed query', function (done) {
53+
test('publishes error on failed query', function (done) {
5854
const client = helper.client()
5955
client.connection.emit('readyForQuery')
6056

@@ -91,7 +87,7 @@ testTracing('query diagnostics channel', function () {
9187
})
9288
})
9389

94-
testTracing('query context includes client info', function (done) {
90+
test('query context includes client info', function (done) {
9591
const client = helper.client({ database: 'testdb', host: 'localhost', port: 5432, user: 'testuser' })
9692
client.connection.emit('readyForQuery')
9793

@@ -124,7 +120,7 @@ testTracing('query diagnostics channel', function () {
124120
client.connection.emit('readyForQuery')
125121
})
126122

127-
testTracing('promise query publishes diagnostics', function (done) {
123+
test('promise query publishes diagnostics', function (done) {
128124
const client = helper.client()
129125
client.connection.emit('readyForQuery')
130126

@@ -158,8 +154,8 @@ testTracing('query diagnostics channel', function () {
158154
})
159155
})
160156

161-
testTracing('connection diagnostics channel', function () {
162-
testTracing('publishes start on connect with callback', function (done) {
157+
test('connection diagnostics channel', function () {
158+
test('publishes start on connect with callback', function (done) {
163159
const Connection = require('../../../lib/connection')
164160
const { Client } = helper
165161

0 commit comments

Comments
 (0)