-
Notifications
You must be signed in to change notification settings - Fork 29
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
Define RequestState
and ResponseState
kinds and integrates them into repo; code cleanup
#86
Conversation
@nsaunders I didn't realize you had requested reviews from others on this yet, so I thought it had gone unnoticed. Thus, I also didn't realize that the CI build failed to pass. Sorry for not updating this PR to fix the build. |
594b6ee
to
62125a7
Compare
ResponseState
kind and integrate it into repo; migrate to SpagoRequestState
and ResponseState
kinds and integrates them into repo; code cleanup
This build is failing because there hasn't been a new package set release that includes the updated |
Would it be possible for you to split out the formatting/Ix.do changes into a separate PR? It's a big chunk of the PR that seems unrelated to the kinds change. Would make it more approachable for reviewing. Thanks! |
291fe12
to
3af45b7
Compare
I ported the do notation change commits to the other PR. Is that all? Or were you referring to other formatting commits, too? |
@owickstrom I'm guessing you've been unable to get to this PR. So, I'm following up again. Were there any other formatting changes I needed to do? |
On reflection, I'm pretty sure this could change is not needed if the |
Sorry for the unresponsiveness. I'll try to get to it this weekend. |
- This is more consistent and focuses on the request and response state, not the component change
As I was using this in my own project, I came across an annoyance... A fully fleshed out Middleware
m
(Conn req reqStateFrom res resStateFrom compFrom)
(Conn req reqStateTo res resStateTo compTo)
a ...can have 8 different transitions
In these recent commits, I moved all of these possible transitions into the
After making that change, I updated the rest of the files in the |
One issue with this PR as it stands is that one could theoretically transition from one state to an invalid one if one defined an invalid type signature. For example foo :: ResponseTransition m req BodyRead BodyUnread res resState comp a
foo = -- implementation I'm not sure what the best way to handle this is because we'd need to use type-level functions to validate a transition. For example, import Prim.TypeError (class Fail, Text)
class ValidRequestTransition (from :: RequestState) (to :: RequestState)
instance validRequestTransitionUnreadToRead
:: ValidRequestTransition BodyUnread BodyRead
else
instance invalidTransition
:: (Fail (Text "Invalid request state transition"))
=> ValidRequestTransition from to
class ValidResponseTransition (from :: ResponseState) (to :: ResponseState)
instance validResponseTransitionStatesLineToHeadersOpen
:: ValidResponseTransition StatusLineOpen HeadersOpen
else instance validResponseTransitionStatesLineToBodyOpen
:: ValidResponseTransition StatusLineOpen BodyOpen
else instance validResponseTransitionStatesLineToResponseEnded
:: ValidResponseTransition StatusLineOpen ResponseEnded
else instance validResponseTransitionHeadersOpenToHeadersOpen
:: ValidResponseTransition HeadersOpen HeadersOpen
else instance validResponseTransitionHeadersOpenToBodyOpen
:: ValidResponseTransition HeadersOpen BodyOpen
else instance validResponseTransitionHeadersOpenToResponseEnded
:: ValidResponseTransition HeadersOpen ResponseEnded
else instance validResponseTransitionBodyOpenToBodyOpen
:: ValidResponseTransition BodyOpen BodyOpen
else instance validResponseTransitionBodyOpenToResponseEnded
:: ValidResponseTransition BodyOpen ResponseEnded
else instance invalidResponseTransition
:: (Fail (Text "Invalid response state transition"))
=> ValidResponseTransition from to
-- | Alias for easily defining both the request and response state transitions,
-- | including a change in the component type.
type ConnTransition'
m
(request :: RequestState -> Type)
(fromRequestState :: RequestState)
(toRequestState :: RequestState)
(response :: ResponseState -> Type)
(fromResponseState :: ResponseState)
(toResponseState :: ResponseState)
fromComp
toComp
a
= ValidRequestTransition fromRequestState toRequestState =>
ValidResponseTransition fromResponseState toResponseState =>
Middleware
m
(Conn request fromRequestState response fromResponseState fromComp)
(Conn request toRequestState response toResponseState toComp)
a Since empty class dictionaries are removed in the latest PS release, this shouldn't incur a performance penalty during runtime. However, since one can define a Middleware outside of the transition type aliases, one could still create a Middleware that permits an invalid state transition. |
Sorry, my time is very limited at the moment to work on this stuff, and it was some time ago so I'm a bit disoriented in the codebase. Pardon my slow response. But from what I can gather, I find this to be a very large change, touching the API a lot and adding a bunch of type parameters and aliases. I think the gained type safety and constraint (with the custom kinds) isn't really worth it, frankly. The old type signatures were simpler and more readable to me. But, importantly, people using this API should weigh in. I'm very much out of the loop on this. Also, I realize this PR might be partially based on my old comment, so apologies if it feels like misleading. But in hindsight, maybe that level of type encoding isn't worth. I dunno. Does this make sense? |
Thanks for spending time looking at this. I don't blame you regarding the 'whether this level of type encoding is worth it or not' comment. I've been partially wondering that myself as I've dealt with compiler errors and whatnot. Moreover, I wasn't able to figure out how to ensure that a request/response state transition is valid. I'm not sure whether that can be done by indexed monads, at least how they are defined right now, due to what I stated in my above comment. Still, you use phantom types for the response type, but don't seem to want to use it for the request type. If so, then why use phantom types for the response type at all? Wouldn't it be better to use |
Fair point. I there might be two separate things going on:
I'm unsure wether (1) is worth it, due to the user mostly dealing with getting the response right (the web server does most of the request processing). The exception being streaming the request body, which could be the thing tipping the scales towards using phantom types (or something else.) Regarding (2), that's what I referred to specifically when talking about the custom kinds and API changes. Sure, you can't then construct a Just to be clear, I'm not saying this design is better or worse than the existing API; it just makes different tradeoffs. Hyper isn't exactly used heavily (I don't know of any "production" apps using it) so the API mustn't be super stable in that regard, but I think this kind of change isnt't worth the API compatibility problems. Thanks for taking the time to work on this! |
Yeah, that makes sense. It seems that this (request body streaming) could either be supported out-of-box (use request kinds), or an end-user could opt-in to such a feature if they wanted to support that on their server (define a different Conn type that supports this and allow end-user to reimplement the parts of this library they want).
I thought the change actually made things clearer. When I was first looking at this repo and looked at the type signature for things, it didn't immediately stand out to me that Still, I see the tradeoff of the verbosity. While the kind annotation in functions could be dropped (e.g.
Yup! Looks like the tradeoff is more verbosity for higher type-safety. You do say this library is an experimental approach, so that might also be why it's not used in production AFAWK. And yeah, this would be a breaking change. I've spent the last few weeks looking at the different approaches one can use in PureScript for a web server. Personally, I think an even simpler implementation for a web server should be built. Then, one can choose their design tradeoffs by choosing frameworks that build upon that base implementation (e.g. choose the approach of |
Fixes #85. In this process, I also had to create a
WriterResponse
newtype to implementgetWriter
.Fixes #48
Addresses part of #42, but doesn't implement the real solution.
Also converts Node's
HttpResponse
from a data type to a newtype.(Yes, I realize I should have opened an issue sooner before working on this, but I was curious to learn more about this repo and this work seemed like a good way to get more familiar)