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

Update docs related to hoistServer #824

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

phadej
Copy link
Contributor

@phadej phadej commented Oct 1, 2017

No description provided.

@phadej phadej requested a review from alpmestan October 1, 2017 16:21
@phadej phadej force-pushed the hoist-server-docs branch from 0479fe4 to 15cc4f5 Compare October 1, 2017 17:24
```

We unfortunately can't use `readerServerT` as an argument of `serve`, because
`serve` wants a `Server ReaderAPI`, i.e., with handlers running in `Handler`. But there's a simple solution to this.

### Enter `enter`
### Welcome `hoistServer`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, "Hoistserver hoistServer" and "Welcome welcome" just don't have that ring to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used some time, but I didn't come up with any pun on hoist :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up with hoist

Copy link
Contributor

@alpmestan alpmestan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy enough with this. If there's anything confusing for some users, we can just expand on whatever's not clear enough then.

@phadej phadej merged commit 6e431d5 into haskell-servant:master Oct 2, 2017
@phadej phadej deleted the hoist-server-docs branch October 2, 2017 06:27
@phadej phadej added this to the 0.12 milestone Nov 6, 2017
@dbaynard
Copy link

dbaynard commented Dec 6, 2017

Was going to add this at #804 but it may be better here, as the documentation doesn't quite cover all I needed to replace the use of enter in my code.

I've just adapted a server, and I'm having to rewrite all my API types. This change means my handlers can no longer have type annotations like a -> Handler a or a -> (Handler a :<|> b -> Handler a) but must instead have the annotation Server api or ServerT api Handler. Is this correct?

The enforced use of the type family is causing difficulties. With this change, the standard way of joining fragments of an API, using :<|>, is painful, as each endpoint must be given its own type in order to supply to the new style handler functions. An approach like servant-generic seems necessary.

So given the following example lifted almost directly out of the aforementioned server code (please excuse the relative cruft; I've just got it compiling with the changes),

  1. Is there still a way to write and supply such handlers which doesn't require knowledge of API endpoints?
  2. How should one structure the whole API, now that individual parts must be provided to hoistServer?

In addition, hoistServerWithContext should probably be mentioned in the docs, in the context of Context.


The API for pages to be served is fairly straightforward. CRUD is a type synonym. With this change, the endpoints corresponding to handlers which are not themselves simple functions in Handler must be referenced later, and so need their own type synonyms. This makes for a relatively unpleasant specification.

type PageAPI
     = "index" :> Capture "page" Page :> Get '[HTML] Page
-  :<|> "temp_pages" :> "socials" :> "dinner" :> CRUD "eventid" SocialEvent BookingAmend ("baId" `Tagged` Text) Signup
+  :<|> "temp_pages" :> "socials" :> "dinner" :> DinnerCRUD
-  :<|> "admin" :> "dinner" :> Capture "eventid" SocialEvent :> "diets" :> Get '[HTML,JSON] DinnerDiets
+  :<|> "admin" :> "dinner" :> DietsEndpoint
  :<|> "stylesheets" :> Capture "css" StyleSheet :> Get '[CSS] Css
+
+type DinnerCRUD = CRUD "eventid" SocialEvent BookingAmend ("baId" `Tagged` Text) Signup
+type DietsEndpoint = Capture "eventid" SocialEvent :> "diets" :> Get '[HTML,JSON] DinnerDiets

These handlers requiring natural transformations now need to be provided with API endpoints.

 pageHandler :: Server PageAPI
 pageHandler = pure
-  :<|> serveDinner
+  :<|> serveDinner @DinnerCRUD @'[]
-  :<|> serveDiets
+  :<|> serveDiets @DietsEndpoint @'[]
   :<|> serveCss

In this example, such handlers must interface with the database (accessed using persistent). Such access is provided with the following abstraction. But the migration from enter to hoistServerWithContext means all uses now require an api.

-serveBackend :: ∀ backend a b . backend -> (a -> ReaderT backend IO b) -> a -> Handler b
-serveBackend backend = enter $ transformDB backend
+serveBackend
+  :: ∀ api context backend .
+    HasServer api context
+  => backend
+  -> ServerT api (ReaderT backend IO)
+  -> ServerT api Handler
+serveBackend backend = hoistServerWithContext (Proxy @api) (Proxy @context) $
+  transformDB backend

-transformDB :: ∀ backend . backend -> ReaderT backend IO :~> Handler
-transformDB backend = NT $ liftIO . flip runReaderT backend
+transformDB :: ∀ backend a . backend -> ReaderT backend IO a -> Handler a
+transformDB backend = liftIO . flip runReaderT backend

I find it problematic that the api (and also context) must be passed through even though there's no associated meaning.

The serveDinner handler is more complicated than necessary to display this problem; this handler displays the same difficulties. (readToUnknown is from persistent. And backend would typically be supplied from server settings.)

 serveDiets
-  :: SocialEvent
-  -> Handler DinnerDiets
-serveDiets = serveBackend (readToUnknown . diets)
+  :: ∀ api context .
+    ( HasServer api context
+    , ServerT api (ReaderT SqlBackend IO)
+      ~ (SocialEvent -> ReaderT SqlBackend IO DinnerDiets)
+    )
+  => ServerT api Handler
+serveDiets = serveBackend @api @context backend (readToUnknown . diets)

 diets
   :: ∀ m . MonadIO m
   => SocialEvent
   -> ReaderT SqlReadBackend m DinnerDiets

(Implementation of diets elided).

@alpmestan
Copy link
Contributor

@phadej The changes @dbaynard made are not mandatory right?

@dbaynard
Copy link

dbaynard commented Dec 11, 2017

I believe the issue is I'd been converting everything to operate in Handler, whereas this change supports an approach where everything operates in a ReaderT Environment IO context that is converted to Handler only once, at the top level.

I'll probably have to refactor anyway as that is probably a more flexible approach, but it is not the approach reflected in the docs.

Edit: Also servant-generic doesn't help here.

@alpmestan
Copy link
Contributor

alpmestan commented Dec 12, 2017

@dbaynard I think it is (but maybe not clearly enough?) covered in this section of the tutorial, where handlers live in some monad that's not Handler and hoistServer is used at the very end of the pipeline, to "bring them back in Handler" before we serve those handlers. Unless I'm misunderstanding what you said above. I do believe you really can do without all those messy types etc. If you need any help figuring out how to achieve this, feel free to point us to some complete "minimal example" where such troubles appear or to a repo with your actual code, if it's public.

@dbaynard
Copy link

I believe you understand me correctly. I'm going to refactor, and then I may have feedback on the tutorial. My first instinct is that the section you linked should immediately follow the section about to Handler, though it may be that I can contribute something more constructive.

@alpmestan
Copy link
Contributor

alpmestan commented Dec 13, 2017

Hmm that would make sense. The current structure is mostly due to me wanting the reader to get a good grip on Handler before doing anything fancier that involves other monads. I don't know whether that's better than what you suggest or not, thoughts? Regardless, if you find some explanations to be missing, we very gladly take tutorial patches too :)

@dbaynard
Copy link

I suspect ‘Using another monad for your handlers’ could fit immediately before ‘response headers’ without compromising such an intuition for Handler.

In learning to use the servant ecosystem, I began (following the tutorial) by implementing handlers that returned values with no associated actions (using pure), then progressed to serving html, and eventually needed to interact with databases. It was not obvious at the time that what I needed to do was provide a natural transformation, but once I found the relevant section of the docs it was clearly the path to follow. It also makes good sense: transforming the context should not affect the values and this is reflected in the types.

From speaking with other developers and reading articles and code it seems very common in larger, more complicated servers for the entire set of handlers to operate in some sort of Reader environment. Now I have little experience with web servers, which may explain my naïvety here, yet I found this is neither covered in the servant tutorial nor the examples. With such insight it's clear to me I've drawn abstraction barriers in the wrong places in my code, and this change (enter to hoistServer) has forced me to address it.

So I'll bear all this in mind as I make these changes, and will happily contribute to documentation once I figure it out.

@phadej
Copy link
Contributor Author

phadej commented Dec 13, 2017

@dbaynard do I understand you right, the section I wrote in this PR is good (it mentions Reader after all), but should be introduced earlier in the tutorial?

@dbaynard
Copy link

Yes - at least for me. It's clear on how to transform between monads but until I needed to do so it wasn't clear why I'd want to do that. Directly following the Handler section makes it look like the idiomatic way to add extra functionality.

Actually, the haddocks for hoistServer are clearer than the tutorial on why one might need to use a monad other than Handler.

Once I can identify what wasn't obvious, I'll make a docs PR, then.

@alpmestan
Copy link
Contributor

A quick, barely related note: we'll soon have the beginning of a cookbook up on readthedocs, which hopefully will clearly illustrate in greater detail how to do a bunch of things like the ones you were looking to do but couldn't figure out how. See #867 for more.

@dbaynard
Copy link

Just a heads up — I had some time to work on the project to which I referred above, and completed the refactor. hoistServer (with the ServerT associated type family) is very elegant.

With hindsight, the following things would have helped, at the time.

  1. Working in a custom monad and the using hoistServerWithContext once, at the top level. I hadn't quite appreciated the fundamental power of :<|> — that it connects any two types — as almost all the examples use it to combine values of type Server _ or Handler _.

    Actually, even just using ReaderT SqlBackend IO in the example above allowed me to get rid of all the messy type annotations. So the 'custom monad' can be somewhat ad-hoc.

  2. It wasn't clear to use hoistServerWithContext. Searching readthedocs just now came up empty. I've seen a number (a question on use hoistServer with AuthProtect #906, Issue with switching to 0.12's hoistServer servant-auth#82) of issues relating to this.

  3. I tried following some advice, throwing ServantErr exceptions to IO, and it helped clarify the code. I then ran into runtime errors (that were easy to solve — just need to catch the exceptions in the natural transformation supplied to hoistServer*). I quite support Redirect combinator ? #117 as a result.

So I'll submit a documentation PR (likely including the cookbook, too). Keep up the good work!

@alpmestan
Copy link
Contributor

So I'll submit a documentation PR (likely including the cookbook, too). Keep up the good work!

Lovely, thanks in advance for that!

@phadej
Copy link
Contributor Author

phadej commented Feb 27, 2018

Related 3rd point: Hmm. I'm personally 👎 on FPCo's exception advice. It's not something community at large agrees to be a good general practice (Handler is ExceptT ServantErr IO for a reason).

EDIT: OTOH please share the recipe, we can make it better when we see it!

@alpmestan
Copy link
Contributor

Agreed for exceptions, the less I deal with them the better, but servant will never make that decision for users anyway, so we don't have to agree on this :)

@domenkozar
Copy link
Contributor

my 2c: docs should use ReaderT with IO since it's more practical :)

@alpmestan
Copy link
Contributor

I'd like to disagree strongly but most people do seem to be working with an application monad with a ReaderT in it... =)

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.

5 participants