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

Split off App into its own file #197

Open
MaxGabriel opened this issue Dec 27, 2019 · 3 comments
Open

Split off App into its own file #197

MaxGabriel opened this issue Dec 27, 2019 · 3 comments

Comments

@MaxGabriel
Copy link
Member

Right now App is at the top of Foundation:

-- | The foundation datatype for your application. This can be a good place to
-- keep settings and values requiring initialization before your application
-- starts running, such as database connections. Every handler will have
-- access to the data present here.
data App = App
{ appSettings :: AppSettings
, appStatic :: Static -- ^ Settings for static file serving.
, appConnPool :: ConnectionPool -- ^ Database connection pool.
, appHttpManager :: Manager
, appLogger :: Logger
}

Something 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 from AppSettings. This creates a bad situation, where these functions must be placed in Foundation itself, to get access to App and then be used in instance Yesod App. Here are some example development scenarios:

  1. 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 of App is that all this useful stuff is in a central datatype.

  2. Alternatively, you can imagine you start off with the middleware needing App from the start and being in Foundation. 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.

  3. You have some utility functions that use Handler, which you build up in a file. Then later you want to use some of these in Foundation. 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

  1. The premise of scaffolding is that it's a good architecture to grow your application around. I believe App in Foundation 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 in Foundation.

Orphan instance

Having the Yesod typeclass in a separate file from App 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?

@snoyberg
Copy link
Member

Seems reasonable, no objection from me.

MaxGabriel added a commit that referenced this issue Dec 29, 2019
Detailed reasoning in #197, and in the code comments of this PR

Closes #197
@parsonsmatt
Copy link

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.

@MaxGabriel
Copy link
Member Author

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?

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

No branches or pull requests

3 participants