DX improvments by adding typehints #356
Conversation
| auth?: { | ||
| headers: Record<string, string> | ||
| }, | ||
| userAuthentication?: { |
There was a problem hiding this comment.
Haven't set up fields that are isn't actually referenced in this package. For service specific options the user can import typing from the service's package and use an intersection of types.
There was a problem hiding this comment.
In fact authEndpoint for example is deprecated in pusher.
| /** | ||
| * Listen for an event on a channel instance. | ||
| */ | ||
| abstract listen(name: string, event: string, callback: CallableFunction): Channel; |
There was a problem hiding this comment.
This seems to have been present in all supplied connectors and this would be a basic requirement/naming for any different provider's connector.
| /** | ||
| * Get a private encrypted channel instance by name. | ||
| */ | ||
| encryptedPrivateChannel(name: string): NullPrivateChannel { |
There was a problem hiding this comment.
Added so it's has better feature parity with the pusher connector.
There was a problem hiding this comment.
Not sure what the socket.io equivalent...
| @@ -50,7 +50,7 @@ export class SocketIoConnector extends Connector { | |||
| /** | |||
| * Listen for an event on a channel instance. | |||
| */ | |||
| listen(name: string, event: string, callback: Function): SocketIoChannel { | |||
| listen(name: string, event: string, callback: CallableFunction): SocketIoChannel { | |||
There was a problem hiding this comment.
has better typing when it comes to call, bind, apply
There was a problem hiding this comment.
I can update the rest of the package to CallableFunction if desired.
|
|
||
| /** | ||
| * This class is the primary API for interacting with broadcasting. | ||
| */ | ||
| export default class Echo { | ||
| export default class Echo<O extends Record<PropertyKey, unknown>> { |
There was a problem hiding this comment.
user can pass in the typings from providers such as pusher
| @@ -46,6 +51,8 @@ export default class Echo { | |||
| this.connector = new NullConnector(this.options); | |||
| } else if (typeof this.options.broadcaster == 'function') { | |||
| this.connector = new this.options.broadcaster(this.options); | |||
| } else { | |||
| throw new Error('Broadcaster option not set when initialising Echo.'); | |||
There was a problem hiding this comment.
This is a bit nicer than the user calling a method on undefined as the connector has not been set.
| @@ -95,7 +102,7 @@ export default class Echo { | |||
| * Get a private encrypted channel instance by name. | |||
| */ | |||
| encryptedPrivate(channel: string): Channel { | |||
| return this.connector.encryptedPrivateChannel(channel); | |||
| return (this.connector as NullConnector | PusherConnector).encryptedPrivateChannel(channel); | |||
There was a problem hiding this comment.
Not sure why this is here socket.io connector is not implementing it. (The user can always just access echo.connector)
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions! If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response. |
|
@taylorotwell |
|
@jessarcher Any chance this could be looked at again? |
This is a non-breaking change.
This will help users set up quicker by typehinting options.
There is no type testing set up nor there is much to test given no features have been added.
Please see comments below.
I would version this as a minor given the error added and the method on the null channel.