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

Test for faye/websocket-driver-node#8 #29

Merged
merged 1 commit into from
Sep 7, 2014

Conversation

glasser
Copy link
Contributor

@glasser glasser commented Aug 15, 2014

If you decide to open a client connection but then close it before the
TCP connection succeeds, the websocket-driver change tested by this
commit will ensure that it will actually close, rather than ignoring the
close() method.

The change to API.close also seems correct to me, though I'm not sure
if there are any observable changes if you don't do it.

@glasser
Copy link
Contributor Author

glasser commented Aug 15, 2014

This requires faye/websocket-driver-node#8.

@jcoglan
Copy link
Collaborator

jcoglan commented Aug 17, 2014

I've not reviewed these changes by eye yet but I have run them past the Autobahn test suite and there don't seem to be any regressions.

If you decide to open a client connection but then close it before the
TCP connection succeeds, the websocket-driver change tested by this
commit will ensure that it will actually close, rather than ignoring the
close() method.

The change to API.close also seems correct to me, though I'm not sure
if there are any observable changes if you don't do it.
@jcoglan jcoglan merged commit 308f930 into faye:master Sep 7, 2014
@glasser
Copy link
Contributor Author

glasser commented Sep 12, 2014

Thanks for merging my PRs.

You haven't actually released the change to websocket-driver yet, and you haven't changed this module's package.json to pull in that fix, so the test suite on this module now fails.

@jcoglan
Copy link
Collaborator

jcoglan commented Oct 4, 2014

New releases of websocket-driver and faye-websocket are now available. Thanks for the patches :)

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.

2 participants