search

ratchetphp / Pawl

Asynchronous WebSocket client
MIT License
584 stars 85 forks source link

Document Psr\Http\Message\UriInterface as an allowed type for $url args #125

Closed mbabker closed 2 years ago

mbabker commented 3 years ago

In places where a URL is expected, the doc blocks state that strings are required, but there is no type enforcement at the moment. Thanks to the implementation details of GuzzleHttp\Psr7\uri_for() (or its replacement method in their 2.0 release), any Psr\Http\Message\UriInterface is also acceptable in the Pawl API. So, this PR updates the doc blocks to note that both strings and Psr\Http\Message\UriInterface objects are acceptable types when a URL is expected.

mbabker commented 3 years ago

I'd say that since you can already slip through a UriInterface since there isn't strict type checking in place, it doesn't necessarily hurt anything to officially support it. Beyond that, you end up wasting a few CPU cycles building the URL as a UriInterface (which I do in an app to make a couple of dynamic bits easier to deal with), then casting it to a string, only for this library to convert it to a new UriInterface a little later on.

All in all, I'd call it a mild DX improvement since ultimately the Connector internals are going to be working with a UriInterface anyway.

clue commented 3 years ago

I'd say that since you can already slip through a UriInterface since there isn't strict type checking in place, it doesn't necessarily hurt anything to officially support it.

This project currently uses a docbloc type hint, so passing anything other than a string is prohibited and might lead to unexpected behavior. Static analysis tools such as phpstan or even your IDE will warn you against such usage. This project currently targets PHP 5.4+ which is why it doesn't use PHP's primitive type declarations introduced with PHP 7.0. If we really want to use union type declarations, we would have to target PHP 8.0 (which I would argue would be a bit premature considering the current roadmap).

Beyond that, you end up wasting a few CPU cycles building the URL as a UriInterface (which I do in an app to make a couple of dynamic bits easier to deal with), then casting it to a string, only for this library to convert it to a new UriInterface a little later on.

Fair point, but I would argue this is an implementation detail that might change in the future (see #2) and even now the overhead of creating this object is negligible because it's not something that would execute on your critical path.

Very much appreciate the constructive discussion by the way and keep in mind I'm not responsible for maintaining this project, so take my input with a grain of salt. I'm currently looking into updating a few aspects of this project and seeing if this project could take advantage of a more JavaScript-like API (WebSocket API) and I would argue that supporting a wider variety of types for its API might lead to a more confusing API in the long run.