framework
framework copied to clipboard
laravel
[9.x] Add new and improved AblyBroadcaster
Note: this PR is one of four parallel pull requests to add first class support for Ably to Laravel. Please find the other PRs here:
- laravel/docs#8120
- laravel/laravel#5969
- laravel/echo#351
Background: the current Ably broadcaster uses the Pusher JavaScript SDK and the documentation instructs users to use Ably with the Pusher compatibility mode enabled. We’ve received a lot of feedback that various features of the Ably Laravel integration don’t work as expected, often caused by the fact that there’s a dependency on the Pusher JavaScript SDK which we have no control over. This collection of PRs adds a new broadcaster which uses the ably-js SDK which means that we can ensure that the Ably broadcaster will remain stable. We are more than happy to take feedback and answer any questions about the work presented here :)
- Adds a new AblyBroadcaster (enabled by the
ABLY_NATIVE_CLIENTenvironment variable) class compatible with the AblyConnector from laravel/echo#351 and associated tests. Makes it possible to use Ably capabilities when authenticating clients to specify which operations they can perform on specified channels. - See laravel/docs#8120 for usage instructions.
So when people update their Ably stuff will stop working unless they opt-in to the Deprecated Ably broadcaster?
So when people update their Ably stuff will stop working unless they opt-in to the Deprecated Ably broadcaster?
Yes, it's partially true though. Since it fixes some old protocol compatibility issues, the new version uses official ably-js (low latency, guaranteed message delivery, fallbacks ) and you only need to set ABLY_PUSHER_ADAPTER to true.
If preferred, we could change it so that the ABLY_PUSHER_ADAPTER setting is true by default for 9.x ( So, no issues with update) and make the breaking change separately as a 10.x/master PR.
I think we would definitely have to make the old provider the default on a 9.x patch release.
I think we would definitely have to make the old provider the default on a 9.x patch release.
Updated with default use of pusher based AblyBroadcaster. So, no more breaking changes : )
One thing that is stopping me from reviewing this PR is the way the existing AblyBroadcaster was modified. It makes it impossible to read through that large diff.
I would suggest making the "new" / dedicated first-party AblyBroadcaster the entirely new file... something like AblyNativeBroadcaster ... that way that file shows as entirely new instead of the "deprecated" broadcaster being the new file and a big mangled diff on the existing broadcaster that is hard to follow and reason about.
I would suggest making the "new" / dedicated first-party AblyBroadcaster the entirely new file... something like AblyNativeBroadcaster ... that way that file shows as entirely new instead of the "deprecated" broadcaster being the new file
Sure, we will do it 👍
One thing that is stopping me from reviewing this PR is the way the existing AblyBroadcaster was modified. It makes it impossible to read through that large diff.
I would suggest making the "new" / dedicated first-party AblyBroadcaster the entirely new file... something like AblyNativeBroadcaster ... that way that file shows as entirely new instead of the "deprecated" broadcaster being the new file and a big mangled diff on the existing broadcaster that is hard to follow and reason about.
@taylorotwell Updated as per suggestions!
Not at the moment - it's a big PR and I have limited time to review it atm. Plus the corresponding frontend PR is large and quite complex.
@sacOO7 can you maybe rebase with 9.x? Then PHP 8.2 tests can run as well.
@sacOO7 can you maybe rebase with 9.x? Then PHP 8.2 tests can run as well.
Sure, we will do it 👍
@sacOO7 can you maybe rebase with 9.x? Then PHP 8.2 tests can run as well.
Updated! @driesvints Can you please review the PR? We have put a good amount of work to make this better.
@sacOO7 can you maybe rebase with 9.x? Then PHP 8.2 tests can run as well.
Sure, we will do it 👍
It seems you need to re-run tests since GitHub action CI was unable to setup tests for php version 8.2.
Btw, if you have enough time @driesvints, can you do a basic review for https://github.com/laravel/echo/pull/351? It's an independent yet big PR that addresses core compatibility for AblyBroadcaster from the client side
Thanks @sacOO7. I'm probably not the best person to review that as my JS skills aren't really that good for it. I also don't know too much about Ably itself.
Just a quick tip, you can install Laravel Pint globally composer global require laravel/pint and run pint in your root directory in the CLI to have most of the formatting issues solved for you.
To be honest - something that would make my life much easier is if you could release this as a package that adds this as a custom driver.
All you do is call Broadcast::extend('ably-native', function () {...}) where the callback returns your broadcaster that you have here in this PR. I would be willing to help you if you have any questions on how to do that (feel free to email me). Then you can publish your broadcaster on Composer and we can mention it in our official documentation.
Then, we just need to find a way for the client-side stuff to also be an NPM package that you can maintain and that plays well with Echo.
Thanks, @taylorotwell, this really helps! Maybe it's a coincidence, we are trying the same approach. You can check our repository https://github.com/ably/laravel-broadcaster/tree/feature/ably-service-provider. We will surely need your help once this service provider is ready as a composer package. Thanks for the suggestion again!