Skip to content
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

Use pusher driver for laravel reverb #2612

Closed
wants to merge 1 commit into from

Conversation

dstemberger
Copy link

Resolves #2564

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Allow reverb broadcast to reuse the pusher driver

Breaking changes

@spawnia spawnia requested review from stayallive and thekonz December 4, 2024 11:30
@spawnia spawnia added the bug An error within Lighthouse label Dec 4, 2024
Copy link
Collaborator

@thekonz thekonz left a comment

Choose a reason for hiding this comment

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

looks like it should work, but i have to tested it

@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

looks like it should work, but i have to tested it

So should I wait or merge now?

@stayallive
Copy link
Collaborator

stayallive commented Dec 4, 2024

My 2 cents:

I don't really feel like this is really needed... since there is essentially just a single change which is the connection name vs. the current "pusher" driver.

This might be better fitted as a documentation blurb? Otherwise we might need to consider creating one for every Pusher compatible project like Reverb, Socketi, Socket Chief etc. The one thing this has going for it is that it is a official Laravel package of course.

Also Reverb doesn't support web hooks so technically that route isn't needed and cleanup is not happening which is also nice if this is knows. Same issue Laravel WebSockets has.

Not against this change but also not really enthusiastic about it personally.

@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

Also Reverb doesn't support web hooks so technically that route isn't needed

So we could add another method to SubscriptionRouter that does the minimal amount of work for it?

I don't mind adding support for Reverb, as it is an official Laravel package.

@stayallive
Copy link
Collaborator

So we could add another method to SubscriptionRouter that does the minimal amount of work for it?

The route should be removed completely for Reverb, since it will never be called (because Reverb does not have web hooks)

I don't mind adding support for Reverb, as it is an official Laravel package.

Agreed!

@spawnia
Copy link
Collaborator

spawnia commented Dec 9, 2024

I made a bit of a larger overhaul in #2639, please review that.

@spawnia
Copy link
Collaborator

spawnia commented Dec 11, 2024

@spawnia spawnia closed this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error within Lighthouse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GraphQL subscription with Laravel Reverb
4 participants