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

Remove usage of `Free` #8

Merged
merged 7 commits into from Jun 28, 2018

Conversation

Projects
None yet
3 participants
@thomashoneyman
Copy link
Contributor

thomashoneyman commented Jun 25, 2018

@vladciobanu @natefaubion

This PR updates the repo without using Free anymore. In testing it appears to function the same.

The only places that changed non-trivially are in Main and Control.Monad.Example. Take a look at those two files. A brief summary of important changes

  1. We now have a hard-coded Environment and GlobalState type for ExampleM, and all the proper instances without using Free. To be able to write instances I newtyped these, but if you find that tedious, perhaps there's a way to avoid that.
  2. Navigation still happens through the PushType handler, though we could get away with breaking it out into various functions (navigate, showDialog, etc.)

Otherwise this is pretty much the same thing as the original version.

thomashoneyman added some commits Jun 25, 2018

@thomashoneyman

This comment has been minimized.

Copy link
Contributor

thomashoneyman commented Jun 25, 2018

Side note: this does not have updated docs, which I can go back and add in. I left that out until I'm sure this is the approach y'all like!

@natefaubion

This comment has been minimized.

Copy link

natefaubion commented Jun 25, 2018

The compiler somewhat annoyingly does not allow type synonyms in instance heads. You can get around it with a TypeEquals constraint and a coercion.

import Type.Equality as TE

type Environment = { ... }

instance monadAskExampleM :: TE.TypeEquals Environment e => MonadAsk e ExampleM where
  ask = ExampleM (asks TE.from)

In these examples, hardcoding ExampleM in components negates the flexibility that the classes give you. The point of the classes is that you can swap out an implementation by committing to a different type, however, since the components are all monomorphic to ExampleM, you can't swap out a new implementation. You can still swap out a different Environment, which can be enough in many cases, but otherwise you would need to actually reference the classes in the component signatures.

@natefaubion

This comment has been minimized.

Copy link

natefaubion commented Jun 25, 2018

Likewise, in the old examples, the classes are unnecessary because you are using an initial encoding (Free) to swap out different interpreters.

env <- ask
liftEffect $ do
st <- TE.from <$> Ref.read env.state
Ref.write (TE.to $ f st) env.state

This comment has been minimized.

@thomashoneyman

thomashoneyman Jun 26, 2018

Contributor

This bit is a little gross -- I'd prefer to use modify_ -- but couldn't figure out the necessary type coercion.

This comment has been minimized.

@natefaubion

natefaubion Jun 26, 2018

As you've found out, TypeEquals can be very difficult to work with for anything non-trivial. Maybe a newtype is better? You can always export a version that unwraps it. I've done that in the past, and called it expose.

I think in this particular case it would be modify_ (TE.from >>> f >>> TE.to) env.state.

This comment has been minimized.

@thomashoneyman

thomashoneyman Jun 27, 2018

Contributor

I do prefer the type synonym version because it allows people to preserve the way they already tend to write these functions:

env <- ask
st <- getState

Those seem better than...

env <- unwrap <$> ask
env <- expose
st <- unwrap <$> getState

simply because it allows you to stick with patterns you're already used to.


let router' = H.hoist (runExample environment state event.push token) R.component
let router' = H.hoist (flip runExampleM environment) R.component

This comment has been minimized.

@thomashoneyman

thomashoneyman Jun 26, 2018

Contributor

@natefaubion I'm assuming this is the point at which you'd swap in your Test monad with its appropriate instances?

If so...

@vladciobanu perhaps it would be useful for this repository to have both an AppM and a TestM so we could demonstrate why you might want to take this approach. Let me know if you're interested!

@thomashoneyman

This comment has been minimized.

Copy link
Contributor

thomashoneyman commented Jun 26, 2018

Extra notes are above, but I took @natefaubion's advice and used TypeEquals to go back to the type synonym State and Environment.

I also made all components polymorphic over some m with only the relevant type classes they're using.

Still haven't updated the docs, as I'm waiting to finish code first.

@vladciobanu

This comment has been minimized.

Copy link
Owner

vladciobanu commented Jun 26, 2018

I was hoping I'd get some time to look into this today, but I was unable to so far. I should have more time tomorrow.

I really appreciate you taking your time for this PR, thank you.

@vladciobanu

This comment has been minimized.

Copy link
Owner

vladciobanu commented Jun 28, 2018

Finally had a chance to go over it. It looks really good to me, and I love the fact that we're finally able to write components that no longer depend on ExampleM directly.

I think all that remains is docs. My plan would be to merge this as-is and I could update the docs (README and comments) this weekend or early next week, unless you're already in the process of doing so.

@thomashoneyman

This comment has been minimized.

Copy link
Contributor

thomashoneyman commented Jun 28, 2018

Sure, we could do that. I haven't written any docs.

One other thing that I didn't do here, but which might be useful to folks, is to add a Parallel instance for the monad so you can do...parallel things with it. If you don't think that'd be too much I can add it; if that'll just make this even more complex than it is for a beginner to check out, then we could leave it off and just merge this.

@vladciobanu

This comment has been minimized.

Copy link
Owner

vladciobanu commented Jun 28, 2018

I think I would like to merge this as is.

I have not looked at Parallel yet, but that sounds quite interesting. Would you mind opening an issue with a very basic outline of the idea? I'd love to check it out this weekend.

@vladciobanu vladciobanu merged commit 2f0b6a6 into vladciobanu:master Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment