-
Notifications
You must be signed in to change notification settings - Fork 3
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
pathInfo has leading slash as an empty string #4
Comments
Just came across this myself. How did |
Not sure. But, I think that we can just |
Looks like they drop empty strings |
I think Routing Duplex's method is a bit more efficient for parsing all parts of a given path. |
Yea, it seems like they are also parsing query value which is not the purpose of pathInfo. I don't want this lib to have a lot of overhead. That is the reason why I am using something like the P.S: I am assuming now that the URL parsing is more efficient than the PS version. I might be totally wrong. |
Do we really need to provide |
It is somewhat related so I want to point out that for example query parsing is context dependent which can be somewhat surprising. It can depend whether a backend expects given request to be encoded by browser or not (so be prepared to handle space encoding against the spec): |
I think this extra field should be dropped. Perhaps it can be calculated and inserted into the normal request record via |
I fully agree with @JordanMartinez. We can provide utils for quick and opinionated parsing but this should be an opt-in in some way. |
Dumping @paluh and I discussion on slack related to this ticket: paluh 6:45 PM
6:52 Woody 7:03 PM
If we are talking about bare minimum, then an approach like Hyper resonates more with me : https://github.com/purescript-hyper/hyper/blob/master/src/Hyper/Request.purs#L28-L34
Extend it in what way that can help them? Example of what your are thinking would much appreciated
I dont understand the “how much overhead” for example I am using Url module - when I parse the whole url, including queries, everything is already there it’s more of a matter of assign the existing value to different field to give context or make it easy to use as a micro webserver framework in my opinion paluh 7:08 PM Woody 7:21 PM paluh 7:21 PM Woody 7:22 PM paluh 7:23 PM Woody 7:26 PM paluh 7:27 PM Woody 7:28 PM paluh 7:28 PM Woody 7:30 PM paluh 7:31 PM Woody 7:32 PM paluh 7:33 PM Woody 7:33 PM paluh 7:36 PM Woody 7:40 PM paluh 7:42 PM Woody 7:54 PM paluh 7:55 PM Woody 7:55 PM paluh 7:55 PM Woody 7:58 PM paluh 7:58 PM |
Yes, that's exactly the kind of approach that should be taken here. Let's say this framework did not have any url parsing, but just provided the url. To increase performance by not creating a new Record value, one could insert into the run settings \req callback -> app (addMyFields req) callback
where
addMyFields :: NotNewtypedRequest -> ExpandedRequestType
addMyFields req =
Record.insert (SProxy :: SProxy "pathInfo") (parseUrl req.rawPathInfo) req This is something a framework (or even a small function) could do on top of this web server library. It would get you the quick prototyping you might want without forcing those who want their own routing library to pay for the performance cost of calculating what |
I have been looking at this approach but the example that your showing isn't using |
To have mutating request we should start from a builder I think. It could be something like unsafeRequestBuilder :: Builder Unit Request
unsafeRequestBuilder request = unsafeBuilder (const request)
where
unsafeBuilder :: (a -> b) -> Builder a b
unsafeBuilder = unsafeCoerce Of course this would complicate SIDE NOTE: This type Middleware = Application -> Application This can be limiting in the real life - it is really hard to have composable application which doesn't introduce additional context into the stack and not changes the type of the Just to provide a context - on our current backends (currently httpure based) we have a categorical compositions of middlewares (we avoid indexed monads on purpose). type AppM ctx st = ReaderT { | ctx } (StateT { | st} Aff)
type App ctx st = AppM ctx st HTTPure.Response and then we have type Middleware ctx st ctx' st' = App ctx st → App ctx' st'
type Middleware' ctx st = App ctx st → App ctx st This way we can introduce / transform staff like: take configuration and provide db connection, wrap endpoint in transaction, provide state with session which can be modified during request flow etc.: let
apiMiddleware
= handleLogging
<<< handleSession
<<< apiDbMiddleware
<<< dbTransactionMiddleware
<<< reqMetrics
in
case RoutingDuplex.parse Route.route url of
...
Right (Route.Api r') → apiMiddleware $ Api.run r'
... But I think I don't want to introduce this into the wai and these kind of solution could be in contrib and of course in purescript-cookbook :-) |
@Woody88 just showed me that he is starting from something like |
pathInfo property from the
Request
type outputs this["", "path1", "path2"]
for such path/path1/path2
.We do not need that empty string at the beginning there it is best to drop to the forward-slash from the path. Like this, we will end up with something like
["path1", "path2"]
which is cleaner.The text was updated successfully, but these errors were encountered: