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

Subprotocol error when creating connection with ws@8.0.0 #550

Open
benthepoet opened this issue Jul 30, 2021 · 9 comments
Open

Subprotocol error when creating connection with ws@8.0.0 #550

benthepoet opened this issue Jul 30, 2021 · 9 comments
Labels
bug CI-CD Test, build and packaging infra needs-investigation

Comments

@benthepoet
Copy link

benthepoet commented Jul 30, 2021

The 8.0.0 version of ws introduced some breaking changes around protocols. Attempting to create a connection with this version yields the following error.

could not create WAMP transport 'websocket':  SyntaxError: An invalid or duplicated subprotocol was specified
    at initAsClient (C:\Users\BenHanna\source\repos\wamp\node_modules\ws\lib\websocket.js:677:15)
    at new WebSocket (C:\Users\BenHanna\source\repos\wamp\node_modules\ws\lib\websocket.js:80:7)
    at C:\Users\BenHanna\source\repos\wamp\node_modules\autobahn\lib\transport\websocket.js:200:22
    at Factory.create (C:\Users\BenHanna\source\repos\wamp\node_modules\autobahn\lib\transport\websocket.js:288:9)
    at Connection._create_transport (C:\Users\BenHanna\source\repos\wamp\node_modules\autobahn\lib\connection.js:112:44)
    at retry (C:\Users\BenHanna\source\repos\wamp\node_modules\autobahn\lib\connection.js:249:33)
    at Connection.open (C:\Users\BenHanna\source\repos\wamp\node_modules\autobahn\lib\connection.js:383:4)
    at Object.<anonymous> (C:\Users\BenHanna\source\repos\wamp\index.js:65:16)
    at Module._compile (internal/modules/cjs/loader.js:1072:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1101:10)
could not create any WAMP transport

Rolling back to 7.5.3 makes the issue go away.

@oberstet oberstet added bug CI-CD Test, build and packaging infra needs-investigation labels Jul 30, 2021
@venki91
Copy link

venki91 commented Aug 4, 2021

Using >= instead of default ^ in the package.json file is causing the problem. A major version of ws module is getting installed, and it is breaking the autobahn module.
Why are we using >= in package.json? Is there a reason?

@venki91
Copy link

venki91 commented Aug 4, 2021

I see. You are supporting from ws v1.0.

@venki91
Copy link

venki91 commented Aug 4, 2021

Can you put an upper limit to the dependency module versions so that they don't break autobahn in future.
https://semver.npmjs.com/

jszczypk added a commit to jszczypk/autobahn-js that referenced this issue Aug 8, 2021
ws@8 contains breaking changes - crossbario#550
@iamciroja
Copy link

Hello, everyone! Are there any good news on this issue? :)

@gvatsov
Copy link

gvatsov commented Mar 9, 2022

I used the npm-force-resolutions package for my project as a workaround for this issue to point to the non-broken ws version 7.5.3.

@DamiToma
Copy link

DamiToma commented May 4, 2022

I'm not sure what the state of this issue is, but I tested with ws v8.6.0 and the reason it's erroring is that there's a regex test failing. Autobahn passes both JSON and webpack serialization split by a comma, but ws is expecting an array.

I removed the join statement to pass an array and it's working beautifully now. Does it make sense to migrate to v8 and remove that?

@luixo
Copy link

luixo commented May 17, 2022

@oberstet
It seems like you're maintaining the repository
The small change (removing the join and upgrading to the new ws version) can make users' lifes better. If that's too much - the condition may be added to check ws version before joining protocols.
We already have a PR for a quick fix: #552, I can create a PR for a proper one.

@oberstet
Copy link
Contributor

oberstet commented May 17, 2022

just a quick note, what @DamiToma suggests sounds good! @luixo no need for checking for old ws version, we can just bump the minimum version required to v8.6. so yeah, a PR would be awesome, I can merge ..

@DamiToma
Copy link

@oberstet I would suggest to also update the README, as it can be misleading for newbies. Initially I thought it supported up to version 2, we can probably get rid of the comment for Node V4.5.0 (I guess no one has been using that for years).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CI-CD Test, build and packaging infra needs-investigation
Projects
None yet
Development

No branches or pull requests

7 participants