Skip to content

Conversation

@uurien
Copy link
Contributor

@uurien uurien commented Jul 16, 2025

Using Node.js diagnostics_channel, this code returns true:

const diagnostics_channel = require('node:diagnostics_channel');
// const diagnostics_channel = require('dc-polyfill')
const channel = diagnostics_channel.channel('my-channel');
const store = {}

channel.bindStore(store, () => {});

console.log('channel.hasSubscribers', channel.hasSubscribers)

But if node:diagnostics_channel is replaced by dc-polyfill, it returns false in node 18.

This PR want to fix that.

@uurien uurien changed the title Force test failure with bindStore and hasSubscribers in node18 Fix hasSubscribers when it only has bindStore in node <=20 Jul 16, 2025
@uurien uurien changed the title Fix hasSubscribers when it only has bindStore in node <=20 Fix hasSubscribers when channel only has bindStore subscriptions in node <=20 Jul 16, 2025
@uurien uurien marked this pull request as ready for review July 16, 2025 16:08
@uurien uurien requested a review from tlhunter July 16, 2025 16:11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the test-*.spec.js files we basically copy and paste from the Node.js repository, make a few changes to be compatible with the test runner, and then commit that. In theory it will help us backport more tests in the future. But for tests that Node.js probably doesn't care about and we do, we then add a completely new test file to the test/ dir that doesn't have a test-* prefix.

For example this test file is copied from Node here:
https://github.com/nodejs/node/blob/main/test/parallel/test-diagnostics-channel-has-subscribers.js

Instead of adding the new test cases to this Node.js file can you make an entirely new test file?

@simon-id
Copy link
Member

is your PR taking inspiration from node core ? since this looks like it was fixed in node core, we should copy that change, and the test that goes with it

@uurien
Copy link
Contributor Author

uurien commented Jul 17, 2025

is your PR taking inspiration from node core ? since this looks like it was fixed in node core, we should copy that change, and the test that goes with it

node core code for stores is very different, it doesn't need to add support to existing channel, so it works replacing the prototype when a store or subscription is added. Here we are not doing that for stores.

Copy link
Member

@simon-id simon-id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to wait for thomas approval too

@uurien
Copy link
Contributor Author

uurien commented Jul 17, 2025

feel free to wait for thomas approval too

Yes, definitely I'll wait for him

@uurien uurien merged commit edae035 into main Jul 21, 2025
43 checks passed
@uurien uurien deleted the bindstore-hassubscribers branch July 21, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants