search

denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.98k stars 5.23k forks source link

Determining request "upgradability" for web socket before upgrade #11694

Open kitsonk opened 3 years ago

kitsonk commented 3 years ago

The Deno std library for web sockets had a function acceptable() which would determine if a current HTTP request was upgradeable to a web socket. With Deno.upgradeWebSocket() there is nothing you can do to determine the acceptability of a Request to be upgraded definitively.

Currently a user of oak is encountering an issue (oakserver/oak#370) were the std library acceptable() is returning true, but when the socket is attempted to be upgraded it is failing, saying no upgrade is available. (Let alone the error message not providing enough information as to the cause of the inability to upgrade, it is simply saying no upgrade available).

lucacasonato commented 3 years ago

The specific issue (oakserver/oak#370) should be solved by #11675.

kitsonk commented 3 years ago

Not according to the OP. It allegedly is just connection: upgrade.

Still a bit of guesswork if it is upgradeable unless you try to upgrade.

crowlKats commented 3 years ago

The no upgrade available error means something is wrong in our check for valid websocket request and are considering invalid requests as valid ws requests, as that error is coming from hyper in the upgradewebsocket op

kitsonk commented 3 years ago

I understand what it means. If you look at the linked issue, there is no obvious reason why it is "invalid" and the error message give you no clue to narrow it down.

Also, if the only way to find out if something is definitively upgrade is to try to upgrade it, seems like a design limitation, ergo the request for something like we have in std.

crowlKats commented 3 years ago

What is the usecase of wanting to check if it is upgradeable without upgrading it?

kitsonk commented 3 years ago

Specifically in oak, there is isUpgradable in a context (and is acceptable std). This gives better middleware handling when trying to determine how to handle a request, versus having to try to upgrade the connection and catch an error (which should be done anyway, but recovering from an error versus explicit logic to handle when the connection is not upgradeable, is desirable).

At the very least, we are taking away capabilities (integrated into other libraries) from what we offer in std without a good justification of why we are dropping functionality, irrespective of the perceived use case.

crowlKats commented 3 years ago

I see, sounds good to me to have then