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

Change makePendingConnection to accept a Stream instead of a Socket #99

Merged
merged 2 commits into from Apr 30, 2015

Conversation

nilscc
Copy link
Contributor

@nilscc nilscc commented Apr 26, 2015

Hi!

I'd like to propose the following change: Use a Stream argument for makePendingConnection instead of a Socket. This allows the use of more general streams, e.g. for a TLS connection. Currently, this is the easiest and cleanest solution since it is not possible to implement something like makePendingConnection outside of the websockets library because of the decodeRequestHead function, which is a private function from the hidden module Network.Websockets.HTTP.

This change does break the current API, but I think it is a reasonable change and easy to fix for any users of this function (see e.g. the change in runApp).

– McManiaC

@jaspervdj
Copy link
Owner

I get the idea, but I don't like breaking backward compatibility when it is easily avoidable. What you could do here is have two functions:

makePendingConnection :: Socket -> ConnectionOptions -> IO PendingConnection
makePendingConnectionFromStream :: Steam.Stream -> ConnectionOptions -> IO PendingConnection

Then the implementation of the first one is very straightforward: call makeSocketStream and then call makePendingConnectionFromStream.

This ensures user convenience & backward compatibility. :-)

@nilscc
Copy link
Contributor Author

nilscc commented Apr 27, 2015

Fair enough, I added another function and reverted the type of makePendingConnection.

@jaspervdj
Copy link
Owner

Thanks!

jaspervdj added a commit that referenced this pull request Apr 30, 2015
Change makePendingConnection to accept a Stream instead of a Socket
@jaspervdj jaspervdj merged commit 2912ced into jaspervdj:master Apr 30, 2015
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