Skip to content

Conversation

@jonahsnider
Copy link
Contributor

@jonahsnider jonahsnider commented Jul 2, 2024

πŸ”— Linked issue

Closes #65

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Allows adding multiple handlers to a single channel subscription.
The old API will still work. A new API was added for removing specific handlers without fully unsubscribing from the channel:

const handler1 = () => { /* ... */ };

// start subscription
await redis.subscribe('users/123', handler1);

const handler2 = () => { /* ... */ }; 

// use the existing subscription
await redis.subscribe('users/123', handler2);

// remove handler1, keep subscription alive
await redis.unsubscribe('users/123', handler1);

// no more handlers for channel, close subscription
await redis.unsubscribe('users/123', handler2);

To avoid introducing any breaking changes, the errors in errors.ts were marked @deprecated, since they are no longer relevant with the introduction of this feature.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@RomainLanz RomainLanz closed this Jan 31, 2025
@RomainLanz RomainLanz reopened this Jan 31, 2025
@RomainLanz RomainLanz force-pushed the multiple-subscription-handlers branch from 35d3de5 to c3871a7 Compare January 31, 2025 23:25
@RomainLanz
Copy link
Member

RomainLanz commented Jan 31, 2025

Hey @jonahsnider! πŸ‘‹πŸ»

Sorry for the delayβ€”we totally missed this issue. πŸ˜…

I've pushed some changes so the tests can run properly. Now, we just need to update them so they pass with this PR since they were failing before.

Also, I don't think this change is breaking since it adds a feature that wasn't possible before. We throw an error when people tried to do this, so there might be some users expecting that error in their application. What do you think, @thetutlage?

@RomainLanz RomainLanz requested a review from thetutlage February 1, 2025 19:54
@jonahsnider
Copy link
Contributor Author

Hey! No worries with the delay, thanks for taking a look now. I just pushed a change to fix the failing tests, let me know if there are more cases that should be added to the suite. Your point about users maybe expecting the error to be thrown is a little spooky, but hopefully that's not a common use case? Would probably be a good idea to add a callout in any release notes just in case.

@RomainLanz RomainLanz requested a review from Julien-R44 February 2, 2025 10:16
})

test('throw error when subscribing to a channel twice', async ({ cleanup }) => {
test('do not throw error when subscribing to a channel twice', async ({ cleanup }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead write an assertion that both the handlers are being called?

})

test('throw error when subscribing to a pattern twice', async ({ cleanup }) => {
test('do not throw error when subscribing to a pattern twice', async ({ cleanup }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

Copy link
Member

@thetutlage thetutlage left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just left a comment to adjust the tests πŸ‘

@jonahsnider
Copy link
Contributor Author

Looks pretty good. Just left a comment to adjust the tests πŸ‘

Thanks for the review! I just pushed a change which updates the tests based on your feedback. LMK if anything else should be adjusted.

@thetutlage thetutlage merged commit f1c79af into adonisjs:9.x Feb 7, 2025
11 checks passed
@thetutlage
Copy link
Member

Looks good! Thanks for the PR :)

@jonahsnider jonahsnider deleted the multiple-subscription-handlers branch February 7, 2025 07:34
@RomainLanz
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support multiple subscriptions to one Redis channel

3 participants