-
Notifications
You must be signed in to change notification settings - Fork 0
Fix hasSubscribers when channel only has bindStore subscriptions in node <=20 #19
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
Conversation
There was a problem hiding this comment.
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?
|
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. |
simon-id
left a comment
There was a problem hiding this 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
Yes, definitely I'll wait for him |
Using Node.js diagnostics_channel, this code returns
true:But if
node:diagnostics_channelis replaced bydc-polyfill, it returnsfalsein node 18.This PR want to fix that.