Open MaxGabriel opened 4 years ago
Seems reasonable, no objection from me.
I'm opposed to this.
The motivation is totally sound, but there's a better solution - separating out App
(the type of the web application context) and DomainContext
(the type of things you need to do for your domain specifically). DomainContext
can be located somewhere else and imported into Foundation
. In this formulation, App
should remain relatively small, and DomainContext
can get as big and bloated as it needs to be, all while staying independent of any web-config tricks.
So, I thought about it a little bit, and while I think splitting off part of App
into DomainContext
would be helpful for e.g. adding CLI app support to your Yesod app, it doesn't solve the core problem of App
and the output of mkYesodData
being stuff you want to import into other modules, which are then imported to be used by Yesod middleware, the authorization functions, defaultLayout, etc.
The most important part generated is Handler
. Fundamentally, I think you should be able to use Handler
for things like middleware or in your authorization checks. Workarounds like MonadHandler
push unnecessary new typeclasses and constraints on users.
Second, things like Yesod middleware, authorization functions, and defaultLayout are fundamentally more than your DomainContext
—they are your (web) App
. It doesn't make sense that normal Handler functions should have easy access to your App
, but that just as web-appy Yesod middleware or authorization function code get a watered down DomainContext
. Also, if one's DomainContext
was truly limited to non-webapp specific things, it wouldn't include e.g. configuration for middleware (e.g. do I turn on CSRF protection, what hosts do I whitelist), so really things like middleware need the full context of App
.
So, for these reasons I'm still in favor of the PR. Thoughts?
Right now
App
is at the top of Foundation: https://github.com/yesodweb/yesod-scaffold/blob/3378e25e54472b7ea9f691b460ff46e9f9e8382c/src/Foundation.hs#L29-L39Something we've run into is that it helps to split out App into its own file, separate from the Yesod typeclass. I propose doing this in the scaffolding so it becomes a default for Yesod users.
Why it's annoying that App is in Foundation:
Foundation inevitably needs a lot of functionality, since the Yesod typeclass includes Yesod middleware and authorization. This functionality in turn often needs access to the
App
datatype, especially since most customization is driven fromAppSettings
. This creates a bad situation, where these functions must be placed in Foundation itself, to get access toApp
and then be used ininstance Yesod App
. Here are some example development scenarios:You want some Yesod middleware to report exceptions in your application to an exception tracking service. It needs a reference to the exception tracking service value which you store in
App
. So you pass this in. Then you expand it to be configurable and pass in settings. Then you expand it to lookup user metadata from the database so you know the email of users who got the exception, so you pass in a database pool. You can keep passing in parameters, but it gets awkward—the whole idea ofApp
is that all this useful stuff is in a central datatype.Alternatively, you can imagine you start off with the middleware needing
App
from the start and being inFoundation
. But as your middleware gradually grows in functionality, suddenly 200 lines of Foundation is exception tracking code you'd rather be in its own file.You have some utility functions that use
Handler
, which you build up in a file. Then later you want to use some of these inFoundation
. Now you need to move some of your utility functions to Foundation itself, cluttering Foundation and splitting up your utilities.These aren't insurmountable issues, but they are a pain. You can workaround them. But imo this kind of thing is bad for beginners—now small changes might require moving around a bunch of files to resolve circular imports, or learning new typeclasses like
MonadHandler
that usually are only necessary in Yesod itself.Why this should be in the scaffolding
App
inFoundation
sets up Yesod users for pain later when they need to refactor things in and out of Foundation, and poor practices if they end up with too much functionality inFoundation
.Orphan instance
Having the
Yesod
typeclass in a separate file fromApp
creates an orphan instance. Personally, I am not bothered by orphan instances in applications (libraries are more of an issue). I also believe the Yesod typeclass is a bit of a special case, because it includes a lot of functionality that is app-specific, whereas usually typeclasses are usually smaller and generic. I am sure people will complain about this, though.Thoughts on this?