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

Closing the websocket #8

Closed
afcondon opened this issue Feb 12, 2016 · 8 comments · Fixed by #9
Closed

Closing the websocket #8

afcondon opened this issue Feb 12, 2016 · 8 comments · Fixed by #9

Comments

@afcondon
Copy link
Contributor

A very small issue: the example in the repo closes the socket with two parameters, both given as Nothing (and interpreted in the WebSocket.js closeImpl method as mCode and mReason respectively.

But this will always fail to dereference and in any case it seems to me that the spec [1] doesn't actually take any parameters, tho' perhaps an antecedent spec did, i don't know.

[1] http://www.w3.org/TR/2009/WD-websockets-20091222/#websocket

@zudov
Copy link
Owner

zudov commented Feb 12, 2016

Hi.

the spec [1] doesn't actually take any parameters

A more modern version of specification seem to say

void close([Clamp] optional unsigned short code, optional USVString reason);

with a more detailed description of close method here.

tho' perhaps an antecedent spec did

Yes, 2009 might be a bit too old.

But this will always fail to dereference

I'll take a look at this when I wake up, I remember testing the example, but probably I didn't notice that, and adding some automated tests which make sure that it works (e.g. in phantomjs) wouldn't hurt anyway. I'll also try to lookup how far does the browser support goes for this parameters, probably it's actually quite new, hence the failures.

It seems that closeImpl passes undefined to close() when it gets Nothing, not sure if it might cause problems (I wouldn't be too surprised if it can).

@zudov
Copy link
Owner

zudov commented Feb 12, 2016

Depending on internal purescript details (value0) isn't good anyway, so it certainly has to be patched.

@afcondon
Copy link
Contributor Author

Hahaha, okay that's embarassing that i didn't notice that my cursory Google search was to an ancient version of the working document. :-)

thanks for following up!

@zudov
Copy link
Owner

zudov commented Feb 14, 2016

In order to properly fix .value0 thingy we need to first resolve https://github.com/zudov/purescript-undefinable. I made that library, but there were some ideas on IRC about adding it to purescript-nullable or integrating with purescript-foreign, so I am not publishing it before we decide what to do.

@zudov
Copy link
Owner

zudov commented Feb 14, 2016

As of original problem, I can't really reproduce the "always fail to dereference" thingy.

When I run the example, it executes the socket.close Nothing Nothing and after some time receives the CloseEvent with status code 1005, which "Indicates that no status code was provided even though one was expected.".

@afcondon
Copy link
Contributor Author

Maybe it's browser specific? Here's what i see running example in Chrome:
screen shot 2016-02-14 at 22 33 42

@zudov
Copy link
Owner

zudov commented Feb 14, 2016

Ok, I see the problem now. Seems that if you do socket.close(undefined, undefined) in Chrome it would interpret undefined as 0. Doing socket.close() is allright though.

I think I should change the close type to something like:

close :: Maybe CodeAndReason -> Eff _ Unit

I am not yet sure what would be a good type (and name) for CodeAndReason I'll think about it a bit.

@zudov
Copy link
Owner

zudov commented Feb 15, 2016

Please take a look at PR.

@zudov zudov closed this as completed in #9 Jun 12, 2016
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 a pull request may close this issue.

2 participants