amphp / websocket-server

WebSocket component for PHP based on the Amp HTTP server.
MIT License
114 stars 17 forks source link

Update README #13

Closed cspray closed 4 years ago

cspray commented 4 years ago

I also would like to have a clarifying question around the documentation on Websocket::onHandshake, specifically the @return documentation:

@return Promise<Response> Resolve with the given response to accept the connection or resolve with a new Response object to deny the connection.

This phrasing implies that a brand new Response instance needs to be created to deny the connection. However, the example in the README, and I believe the more intuitive API, is for any non-2XX response to deny the connection. I am unsure as to the exact semantics here but I would like to either clarify the Websocket docblocks or the example to be consistent.

cspray commented 4 years ago

I understand that the .gitignore was bothersome and I removed it. However, the bigger piece about my question between the discrepancy in the documentation and the example still needs an answer.

trowski commented 4 years ago

A new instance does not have to be returned. The docs should say that the Response instance can be modified to be a non-101 response to effectively deny the connection, though generally a 4xx response would be appropriate. The docs should make it clear that the response object can also have headers added, such as cookies, if desired.

cspray commented 4 years ago

@trowski @bwoebi Got the tests updated from master to get those passing and addressed all of the feedback. Believe this is good to be merged in. Please let me know if there's anything else I can add.

kelunik commented 4 years ago

Thanks!