Add success return value in diagnostics_channel unsubscribe #40433

simon-id · 2021-10-12T19:16:54Z

This PR adds two things to the `unsubscribe` method in `diagnostics_channel` module:

Add a returned boolean that returns true if a subscriber was actually removed, and false if it was not found

Can be used for example like this:

// this will remove every instance of the subscriber, not just the first one
while (channel.unsubcribe(subscriber)) {}

This is currently not possible without messing with the private channel._subscribers property.

Add the unsubscribe method to non-active channels

Currently if a user tries to use the unsubscribe method on a non-active Channel, it will throw channel.unsubscribe is not a function. This doesn't happen if the channel is an instance of ActiveChannel.
Since the user should not be aware of a difference between (non-active) Channel and ActiveChannel, the unsubscribe method should be present in both.

The text was updated successfully, but these errors were encountered:

targos · 2021-10-13T06:33:10Z

@nodejs/diagnostics

vdeturckheim

LGTM. I am not sure on wether changing the return value of a method is a breaking change (in this context, moving from undefined to boolean)

simon-id · 2021-10-13T14:26:10Z

I am not sure on wether changing the return value of a method is a breaking change (in this context, moving from undefined to boolean)

I didn't think about that, you might be right. But since diagnostics_channel is in Stability: 1 - Experimental, that's not a problem is it ?

vdeturckheim · 2021-10-13T14:28:36Z

@simon-id that seems correct 😄

Flarna · 2021-10-13T14:31:56Z

Currently if a user tries to use the unsubscribe method on a non-active Channel, it will throw channel.unsubscribe is not a function.

Could you add a test for this scenario?

simon-id · 2021-10-13T14:35:02Z

Could you add a test for this scenario?

This case is implicitly tested here I think, or is that not what you mean ?

nodejs-github-bot · 2021-10-13T17:40:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/40290/

mhdawson

LGTM

RafaelGSS

LGTM.

simon-id · 2021-10-19T11:02:01Z

Squashed the changes into two logical commits, and (i think) fixed the author issue. I think the commit queue should work now.

vdeturckheim · 2021-10-19T11:28:32Z

@Flarna good for you if I re-add it to the queue ?

targos · 2021-10-19T11:30:11Z

@vdeturckheim it won't work because there was a push after the last CI run.

nodejs-github-bot · 2021-10-19T12:18:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/40367/

vdeturckheim · 2021-10-19T12:19:31Z

Good catch @targos . I just restarted it

github-actions · 2021-10-19T14:37:54Z

Commit Queue failed

- Loading data for nodejs/node/pull/40433
✔  Done loading data for nodejs/node/pull/40433
----------------------------------- PR info ------------------------------------
Title      Add success return value in diagnostics_channel unsubscribe (#40433)
Author     simon-id  (@simon-id, first-time contributor)
Branch     simon-id:patch-1 -> nodejs:master
Labels     semver-minor, author ready, diagnostics_channel
Commits    2
 - lib: add return value for DC channel.unsubscribe
 - lib: add unsubscribe method to non-active DC channels
Committers 1
 - simon-id 
PR-URL: https://github.com/nodejs/node/pull/40433
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Stephen Belanger 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Michael Dawson 
Reviewed-By: Bryan English 
Reviewed-By: Zijian Liu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40433
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Stephen Belanger 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Michael Dawson 
Reviewed-By: Bryan English 
Reviewed-By: Zijian Liu 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - lib: add return value for DC channel.unsubscribe
   ⚠  - lib: add unsubscribe method to non-active DC channels
   ℹ  This PR was created on Tue, 12 Oct 2021 19:16:54 GMT
   ✔  Approvals: 6
   ✔  - Vladimir de Turckheim (@vdeturckheim): https://github.com/nodejs/node/pull/40433#pullrequestreview-778650881
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/40433#pullrequestreview-778754474
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/40433#pullrequestreview-778766673
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/40433#pullrequestreview-779078155
   ✔  - Bryan English (@bengl): https://github.com/nodejs/node/pull/40433#pullrequestreview-780077439
   ✔  - Zijian Liu (@Lxxyx): https://github.com/nodejs/node/pull/40433#pullrequestreview-780489282
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-10-19T12:18:04Z: https://ci.nodejs.org/job/node-test-pull-request/40367/
- Querying data for job/node-test-pull-request/40367/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1359595185

github-actions · 2021-10-19T14:46:16Z

Landed in 0f78d26...07bbb07

PR-URL: #40433 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Zijian Liu <[email protected]>

vdeturckheim · 2021-10-19T15:40:04Z

Congrats @simon-id for this first PR merged!

PR-URL: #40433 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Zijian Liu <[email protected]>

Notable changes: doc: * add VoltrexMaster to collaborators (voltrexmaster) #40566 esm: * (SEMVER-MINOR) add support for JSON import assertion (Antoine du Hamel) #40250 lib: * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433 * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433 v8: * (SEMVER-MINOR) multi-tenant promise hook api (Stephen Belanger) #39283 PR-URL: TODO

Notable changes: doc: * add VoltrexMaster to collaborators (voltrexmaster) #40566 esm: * (SEMVER-MINOR) add support for JSON import assertion (Antoine du Hamel) #40250 lib: * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433 * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433 v8: * (SEMVER-MINOR) multi-tenant promise hook api (Stephen Belanger) #39283 PR-URL: #40758

nodejs-github-bot added the needs-ci label Oct 12, 2021

simon-id marked this pull request as draft Oct 12, 2021

simon-id force-pushed the patch-1 branch from 7c3156c to 64fdae3 Oct 12, 2021

simon-id changed the title ~~Add unsubscribe method to non-active channels~~ Add success return value in diagnostics_channel unsubscribe Oct 13, 2021

simon-id force-pushed the patch-1 branch from 493707f to abbbcd1 Oct 13, 2021

simon-id marked this pull request as ready for review Oct 13, 2021

vdeturckheim approved these changes Oct 13, 2021

View changes

Qard approved these changes Oct 13, 2021

View changes

Flarna approved these changes Oct 13, 2021

View changes

Flarna added the request-ci label Oct 13, 2021

github-actions bot removed the request-ci label Oct 13, 2021

This comment has been hidden.

Sign in to view

simon-id mentioned this pull request Oct 13, 2021

Mirror modifications from node core PR #40433 simon-id/diagnostics_channel-polyfill#3

Merged

Mesteery added author ready diagnostics_channel and removed needs-ci labels Oct 13, 2021

mhdawson approved these changes Oct 13, 2021

View changes

RafaelGSS approved these changes Oct 13, 2021

View changes

github-actions bot mentioned this pull request Oct 14, 2021

CI Reliability 2021-10-14 nodejs/reliability#86

Open

22 tasks

targos added the semver-minor label Oct 14, 2021

bengl approved these changes Oct 14, 2021

View changes

github-actions bot mentioned this pull request Oct 15, 2021

CI Reliability 2021-10-15 nodejs/reliability#87

Open

23 tasks

Lxxyx approved these changes Oct 15, 2021

View changes

Qard added the commit-queue label Oct 15, 2021

simon-id added 2 commits Oct 19, 2021

lib: add return value for DC channel.unsubscribe

d1b1b08

lib: add unsubscribe method to non-active DC channels

bb93096

simon-id reopened this Oct 19, 2021

vdeturckheim added commit-queue and removed commit-queue-failed labels Oct 19, 2021

github-actions bot added commit-queue-failed and removed commit-queue labels Oct 19, 2021

vdeturckheim approved these changes Oct 19, 2021

View changes

vdeturckheim added commit-queue and removed commit-queue-failed labels Oct 19, 2021

github-actions bot removed the commit-queue label Oct 19, 2021

github-actions bot closed this Oct 19, 2021

Qard added the lts-watch-v14.x label Oct 19, 2021

github-actions bot mentioned this pull request Oct 20, 2021

CI Reliability 2021-10-20 nodejs/reliability#92

Open

27 tasks

targos mentioned this pull request Nov 8, 2021

v17.1.0 release proposal #40758

Merged

nodejs / node Public

Add success return value in diagnostics_channel unsubscribe #40433

Add success return value in diagnostics_channel unsubscribe #40433

simon-id commented Oct 12, 2021 •

edited

Add a returned boolean that returns true if a subscriber was actually removed, and false if it was not found

Add the unsubscribe method to non-active channels

targos commented Oct 13, 2021

vdeturckheim left a comment

simon-id commented Oct 13, 2021

vdeturckheim commented Oct 13, 2021

Flarna commented Oct 13, 2021

simon-id commented Oct 13, 2021

This comment has been hidden.

nodejs-github-bot commented Oct 13, 2021

mhdawson left a comment

RafaelGSS left a comment

simon-id commented Oct 19, 2021

vdeturckheim commented Oct 19, 2021

targos commented Oct 19, 2021

nodejs-github-bot commented Oct 19, 2021

vdeturckheim commented Oct 19, 2021

github-actions bot commented Oct 19, 2021

github-actions bot commented Oct 19, 2021

vdeturckheim commented Oct 19, 2021

nodejs / node Public

Add success return value in diagnostics_channel unsubscribe #40433

Add success return value in diagnostics_channel unsubscribe #40433

Conversation

simon-id commented Oct 12, 2021 • edited

This PR adds two things to the unsubscribe method in diagnostics_channel module:

Add a returned boolean that returns true if a subscriber was actually removed, and false if it was not found

Add the unsubscribe method to non-active channels

targos commented Oct 13, 2021

vdeturckheim left a comment

simon-id commented Oct 13, 2021

vdeturckheim commented Oct 13, 2021

Flarna commented Oct 13, 2021

simon-id commented Oct 13, 2021

This comment has been hidden.

nodejs-github-bot commented Oct 13, 2021

mhdawson left a comment

RafaelGSS left a comment

simon-id commented Oct 19, 2021

vdeturckheim commented Oct 19, 2021

targos commented Oct 19, 2021

nodejs-github-bot commented Oct 19, 2021

vdeturckheim commented Oct 19, 2021

github-actions bot commented Oct 19, 2021

github-actions bot commented Oct 19, 2021

vdeturckheim commented Oct 19, 2021

simon-id commented Oct 12, 2021 •

edited

This PR adds two things to the `unsubscribe` method in `diagnostics_channel` module: