-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add support for proxying to a Unix socket #40
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm overall OK with it, but proper API design will be needed, as well as fixing the build on Windows
@@ -75,7 +77,12 @@ import UnliftIO (MonadIO, liftIO, MonadUnliftIO, | |||
data ProxyDest = ProxyDest | |||
{ pdHost :: !ByteString | |||
, pdPort :: !Int | |||
} deriving (Read, Show, Eq, Ord, Generic) | |||
} | |||
| ProxyDestUnix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this approach, since it creates partial functions as well as breaking existing code that pattern matches. A new data type would be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, could you clarify what you have in mind? Avoiding a breaking change would seem to require a lot of duplication. For example, where we have
rawTcpProxyTo :: MonadIO m => ProxyDest -> AppData -> m ()
I could create a parallel universe of functions like
rawTcpProxyToUnix :: MonadIO m => ProxyDestUnix -> AppData -> m ()
but that would require duplicating most of public proxying functions as well as WaiProxyResponse
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't have a specific recommendation in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you accept a full breaking change, renaming the data type and bumping the version number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would lean towards creating a parallel set of functions.
The crux of this PR is adding a new constructor to the
ProxyDest
type calledProxyDestUnix
, and hooking it up to a call toData.Conduit.Network.Unix.runUnixClient
.Happy to add a test or two if you think this could be mergeable. Thanks!