Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coexisting with other WS handlers is broken by the patch to allow other handlers #21

Open
rjp44 opened this issue Nov 22, 2024 · 1 comment

Comments

@rjp44
Copy link
Contributor

rjp44 commented Nov 22, 2024

This change:
https://github.com/jambonz/node-client-ws/blame/462b0245607a2f23cb6004179b8c70ec9040f541/index.js#L72
Adds back 404ing upgrade requests at paths that this library isn't interested in.

It looks like it was done as part of additional functionality to add the ability to specify external WSServers.

Whilst that functionality is great, it only works for static paths that are known at the time makeService() is initialised and it breaks existing code that installs it's own server.on('upgrade', ...) and processes it's own paths dynamically as this library now overwrites the response headers from those upgrades with a 404 if it has no interest in the path, causing any upgrades apart from its own to fail.

Can we compromise and leave this extra External WSS handling in for implementations that choose to use it, but remove the 404 as I believe this is redundant anyway. If all the upgrade hooks fall through because nobody is interested in starting a WebSocketServer to serve the connection attempt at a path, then the connection is rejected anyway in the httpServer fallback handler.

@davehorton
Copy link
Contributor

hmm, interesting can you make a PR to show your suggested changes?

@rjp44 rjp44 changed the title Coexisting with other WS handlers has is broken by the patch to allow other handlers Coexisting with other WS handlers is broken by the patch to allow other handlers Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants