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

replace ClassyPrelude with RIO in simple #194

Open
wants to merge 2 commits into
base: simple
Choose a base branch
from

Conversation

cdornan
Copy link

@cdornan cdornan commented Jun 16, 2019

CF: yesodweb/yesod#1602
CC: @snoyberg

This template takes simple and replaces classy-prelude-*
with rio.

Some points:

  • http-types, yesod-newsfeed and persistent was added with
    no bounds restrictions — what restrictions should we use
    on these packages?
  • rio >= 0.1.8.0 : is my starting point — can we relax the
    upper bound? What upper bound should we use?
  • To get rio to work with the current stack I am hiding parts of the
    rio logging system (I guess these will be addressed by
    this work).

I am out of time this weekend and I have not had time to build and instantiate the template. I will happily do so later in the week (Friday) if nobody has done so by then.

Copy link
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

That turned out to be a much smaller delta than I expected! I had one comment. If things work out nicely, I'm tempted to make this the default template instead of an alternative.

( module RIO
) where

import RIO
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be possible to provide a HasLogFunc instance on the App type instead.

Copy link
Author

Choose a reason for hiding this comment

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

Let me get back to you n that — probably on Friday.

@cdornan
Copy link
Author

cdornan commented Jun 17, 2019

I was also pleasantly surprised also by how easily it seemed to converge. So we press on...

It looks to me as if this template will have to be strictly for early adopters
until the deeper conversion of Yesod to RIO has been completed. All the
differences are superficial but it looks like there are more than enough of
them to confuse without a special guide for the unwary.

The major issue is that the handler is just not a RIO so application developers
are going to have to apply converters to their RIO based handlers to insert
them into Yesod 1.6.

The converter I have constructed here is based on the one I have written for
my own application is based merely on the `App` foundation, but it should
probably be based on `HandlerData`, to give us the (projected) Yesod 1.7
handlers. (I am happy to do this in the next patch if @snoyberg agrees --
I would expect it to be a straightforward generalisation.)

The other immediate issue is that, AFAICS, the RIO loggers and monad-logger
loggers are incompatible, so the Yesod porcelain will continue to use the latter
while RIO handlers would use the former. (Which works for me, but might come
across as clunky for others.)
@cdornan
Copy link
Author

cdornan commented Jun 28, 2019

@snoyberg I have updated the branch based on your suggestion and a live experiment (in a closed-source context). See the notes on the commit. The TL;DR is that it looks to me like Yesod 1.7 will be needed to get a satisfactory template fit for general developers.

The experiment looks like it might be useful however — you are best placed to judge that — and I am happy to continue hacking for as long as you find it useful.

@@ -1,3 +1,4 @@
/stack.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be included in a template, we'll want the users of the template to check in their stack.yaml file.


- rio >= 0.1.8.0
- http-types
- persistent
Copy link
Member

Choose a reason for hiding this comment

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

This dependency and the next shouldn't be included in an actual simple template. I'm assuming it's here for testing/instances/etc.

@vaclavsvejcar
Copy link

@cdornan @snoyberg Just discovered this PR and it's amazing idea, I'd love to have RIO-based Yesod project template, it would fit the ecosystem really nicely. Do you still plan to give it some time to push it forward?

@cdornan
Copy link
Author

cdornan commented Oct 12, 2020

I love the idea, and I think Michael does too — it is just a matter of finding the time to get it done.

I was hoping we might be able cut some corners but that does not look feasible — we would need to convert the heart of Yesod.

The earliest I could look at it would be towards the end of the year but I am pretty sure I would need some help from Michael.

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.

3 participants