Add routing module for pending connections #100
Conversation
| import Network.WebSockets | ||
| import System.FilePath -- (makeRelative, splitDirectories) | ||
|
|
||
| type WebSocketsRouteTy = |
There was a problem hiding this comment.
I think performance-wise it would be beneficial to have a data here, with strict fields.
It will guarantee that objects are next-to one another in memory: it means accessing them will be faster because they have a higher probability to already be in cache.
| , path | ||
| ) where | ||
|
|
||
| import Control.Applicative |
There was a problem hiding this comment.
Maybe make all imports explicit ? It's a personal preference of mine.
| -- Route by subprotocol | ||
|
|
||
| -- | Route by available subprotocols. If any of the protocols in the header | ||
| -- matches the route will succeed. |
There was a problem hiding this comment.
add a coma between matches and the.
| subprotocol proto w = do | ||
| p <- askPending' | ||
| let reqProto = getRequestSubprotocols . pendingRequest $ p | ||
| if proto `elem` reqProto then |
There was a problem hiding this comment.
I think it's more readable when then is on the next line, on its own
| setProto :: Maybe ByteString -> WebSocketsRoute a -> WebSocketsRoute a | ||
| setProto bs r = WebSocketsRoute $ withReaderT (\(p,_,q) -> (p,bs,q)) (unWebSocketsRoute r) | ||
|
|
||
| setPath :: [String] -> PendingConnection -> PendingConnection |
There was a problem hiding this comment.
Performance-wise, maybe it's preferable to use Text in place of String (here and at other places in this file too)
| path r = do | ||
| pending <- askPending' | ||
| case paths pending of | ||
| (p:ps) | p /= "." -> -- makeRelative turns "/" into ["."] |
There was a problem hiding this comment.
In this whole file, I think it would be beneficial to wrap the path (de-)composition logic, to better separate concerns, and make the code more readable. For example here, instead of p /= "." with a comment as to why we don't compare with "/", I would prefer seeing not empty p or something like that.
| assert =<< runClient "127.0.0.1" port "/trailing/?q=a" (expect "trailing") | ||
| assert =<< runClient "127.0.0.1" port "/trailing?q=a" (expect "no trailing") | ||
| assert =<< runClient "127.0.0.1" port "/full?path=reply" (expect "/full?path=reply") | ||
| assert =<< runClient "127.0.0.1" port "/full/2?path=reply" (expect "/2?path=reply") |
There was a problem hiding this comment.
If it is possible to have / at the right of ?, maybe we should add a test with a path like "/foo/bar?q=ab/cd/e"
|
I'm currently not planning to merge this, as I want to limit the scope of the |
|
@jaspervdj Thanks for the info, I don't need routing right now, but will look into the packages you mention later if needed! |
As discussed in our mails, here is my routing module proposal. The idea is based on happstacks routing module. It is "non intrusive" in that it does not require changes to any of the underlying types (
PendingConnectionorConnection). I also added a couple of test cases for it. I had to however raise the dependency onmtlfrom version>= 2.0to>= 2.2to be able to useAlternativeandMonadPlusinstances for theWebSocketsRoutemonad.Please take a look and let me know what you think.
Also, I am (temporarily) hosting generated haddock documentation at: https://nils.cc/hs/websockets/Network-WebSockets-Routing.html