allow extensible server #5
Conversation
Pull Request Test Coverage Report for Build 33
💛 - Coveralls |
There was a problem hiding this comment.
This project tries to follow the WebSocket standard as closely as possible, and rejecting connections when the client fails to follow the protocol must be done.
You can also add the server param to the decontruscted options object and default it to turbo.createServer
|
so this entire lib is effectively useless if you want to use it in tandem with regular http requests? you would have to put a reverse proxy in, run an http server and a ws server? websocket upgrade requests exist specifically for this reason 😕 also i don't think i can do |
|
I hadn't thought about that scenario, thanks for the idea! Will create some tests and will merge if everything works as expected. |
|
I don't think there's a compelling reason to implement this. I suspect most people are looking for a fast websocket server from this library. Additionally, I've not seen any other websocket server allow this - but please share any that do :). Is it not better to have separation of concerns and spin up a http server for http traffic - with, say, maybe turbo-http, if you like. just my 2 cents <3 |
|
@jacktuck the most used websocket library for node, ws, not only allows this behavior but encourages it. the builtin node |
|
@devsnek You're right, my bad. |
| return this.closeConnection(res, 400); | ||
| const headers = req.getAllHeaders(); | ||
| if (/websocket/i.test(headers.get('Upgrade')) && /upgrade/i.test(headers.get('Connection'))) { | ||
| return this.handleUpgrade(req, res); |
There was a problem hiding this comment.
@devsnek did you test this? From my understanding which may be wrong, this will handle stuff coming from turbo-http and in turn turbo-net. The socket provided by turbo-net is not compatible with the Node.js net.Socket so I think you can't simply use an external server, unless that server is a turbo-http server. Is this the goal? It's not clear from the title.
There was a problem hiding this comment.
yes this only works with a turbo-http server instance
There was a problem hiding this comment.
Can you change those regex to String.prototype.includes()? Otherwise, I agree with @devsnek, because as long as you can attach event listeners and you follow the turbo-http API it should be fine.
There was a problem hiding this comment.
regex test allowed the values to not be there or be weirdly cased. otherwise I have to make sure they are strings and such
There was a problem hiding this comment.
if we have to worry about the string's case, i think the current solution is the fastest? https://jsperf.com/includes-vs-test/1
No description provided.