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

Problem with WebSocket constructor #377

Closed
wants to merge 1 commit into from
Closed

Problem with WebSocket constructor #377

wants to merge 1 commit into from

Conversation

bhandfast
Copy link

I was testing websockets, essentially porting the code to Fable from https://github.com/SuaveIO/suave/blob/master/examples/WebSocket/index.html

I got this error in Chrome 52.0.2743.116:
"Error during WebSocket handshake: Sent non-empty 'Sec-WebSocket-Protocol' header but no response was received"

And the cause seems to be calling the websocket constructor as: new WebSocket(address, null);

If I manually changed it to new WebSocket(address) then it worked correctly.

This PR adds an overload to the constructor to prevent emitting null as the second parameter.

@alfonsogarciacaro
Copy link
Member

Nice catch, thanks a lot for spotting this! Setting optional arguments to null was also causing problems with other APIs (like Web Canvas) and was being dealt with here by removing null args at the end of method and constructor calls (see also this issue). However, as you have noticed, this solution wasn't enough for macros as they're transformed on the JS side of Fable by a custom Babel plugin.

Instead of patching the issue by adding an overload to the bindings, it may be better to solve the issue for all macros, so I've moved the null arg cleaning operation to a plugin happening after the macro resolution: f162062

I'll publish a new version with the fix shortly. Please give it a try and we can use your PR if it's still not working for you.

Thanks a lot for your help!

@bhandfast
Copy link
Author

Thanks, version 0.5.10 solves the issue for me without the overload.

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

Successfully merging this pull request may close these issues.

None yet

2 participants