Added a base monad capable of PerformEvent, PostBuild, and TriggerEvent

[ElvishJerricco]

This pull request adds a base monad ReflexBase capable of most of the functionality offered by the various Reflex typeclasses. In particular, it uses PerformEventT and PostBuildT to enable their corresponding typeclasses, and it uses a new TriggerEventT monad transformer for the TriggerEvent class. The motivation of this pull request is to enable users of reflex to write reactive programs in contexts other than web apps with the same relative ease. Having one monad with these three main classes on top of SpiderHost covers a large surface area of requirements.

TriggerEventT was factored out of ImmediateDomBuilder from reflex-dom. As such, much of the Widget monad can be replaced with ReflexBase. This includes most of the implementation of attachWidget, which was factored out into runReflexBase. I haven't changed reflex-dom to make use of this refactoring, but it shouldn't be difficult.

There is one breaking change. The TriggerEvent class now exists in its own module, instead of sitting in the Reflex.PerformEvent.Class module. This is for clarity, not a requirement.

[ElvishJerricco]

Most of the formatting is Hindent -> sanity checks. Let me know if I should reformat.

[ElvishJerricco]

Here's an example project using this.

[mightybyte]

Very cool example

[ElvishJerricco]

Looks like some pretty serious changes were just merged in. Having a hard time seeing how to change this PR to accommodate that.

Also, it looks like there are a lot of classes that aren't instantiated on a lot of types. Is this a problem?

[ElvishJerricco]

To clarify the last part of that last comment: There are lots of MonadX classes that have to do with Reflex. I believe that most (all?) of these can be lifted to have instances on most (all?) of the concrete transformers in Reflex. If so, it seems important to have these instances for the sake of transformer composability.

[ryantrinkle]

Yes, that's a great point! Right now we have all the instances that reflex-dom needs, but it would be great to have the complete set. Are there any automated ways to figure out which ones might be missing? That would be pretty slick.

[ryantrinkle]

Also, on updating the PR, is there any change in particular you're having trouble working through? I'd expect most of these changes to be compatible with what you're doing, at least at the idea level.

[ElvishJerricco]

I doubt that's something one could automate easily. It's the n^2 instances problem, which is just a notorious pain.

I've got a fix that compiles and will probably work. Just a little confused on the introduction of SpiderTimelineEnv and HasSpiderTimeline. The HasSpiderTimeline thing makes it seem to be impossible to make this stuff agnostic from Global anymore.

[ElvishJerricco]

Yea, the runReflexBase function would need to change to use runSpiderHost instead of runSpiderHostForTimeline, meaning users can't manage timelines themselves. Granted, it seems somewhat obscure that a user would truly need a timeline distinct from the global one. But it also seems somewhat obscure that the global one should be the only one usable.

I can't help but feel like the HasSpiderTimeline x class could be replaced with a permeation of ReaderT (SpiderTimelineEnv x), which would allow for non-global timelines (with potentially better inlining?). What's wrong with this intuition?

[ryantrinkle]

In some perf testing I did, ReaderT performed worse than a typeclass - I think it may present fewer opportunities for specialization. The idea with HasSpiderTimeline is that we would construct it using a Kmett-like "reflection" approach. I guess I didn't get around to actually doing that yet, though! What do you think of that concept?

[ElvishJerricco]

Hm weird. Never would have guessed that ReaderT would be slower. Though, withSpiderTimeline would likely be a lot worse with the typeclass, since GHC is pretty bad at figuring out how to specialize when there are constraints in a rank-n type.

Anyway, I'm not familiar with how to do the reflection style approach. Do you know any examples I could look into?

[ryantrinkle]

Yeah, I'm not sure about the perf for dynamically created SpiderTimelineEnvs - the common case for most applications is the Global case, so I optimized for that first. The "reflection approach" is basically to use this to dynamically generate an instance of HasSpiderTimeline.

[ElvishJerricco]

After exploring the reflection package a little, I don't think reflection is a reasonable solution to this problem. We'd want something of this form:

newtype ReifiedSpiderTimeline x = ReifiedSpiderTimeline (SpiderTimelineEnv x)

instance Reifies s (ReifiedSpiderTimeline x) => HasSpiderTimeline x where
  spiderTimeline = let ReifiedSpiderTimeline env = reflect (Proxy :: Proxy s) in env

This is kind of a nonsense instance, and I don't think it can be implemented. I think s needs to appear in the instance declaration, outside of the constraints section, in order to use s in the implementation. Which it obviously doesn't. The implementation of the method as written there doesn't compile.

And of course this instance wouldn't work

instance Reifies s (ReifiedSpiderTimeline s) => HasSpiderTimeline s where
  spiderTimeline = let ReifiedSpiderTimeline env = reflect (Proxy :: Proxy s) in env

because making use of the instance would require pinning s outside of the reify call, which obviously isn't possible. It may be possible if we're willing to unsafeCoerce a SpiderTimelineEnv x to SpiderTimelineEnv (TagType x s), since that can be used to introduce s within the scope of reification. But I haven't tried to work out an implementation of this yet.

Ultimately, if the performance difference is small enough, I'd advocate for the ReaderT based implementation, if for no other reasons than maintainability and mental overhead.

[ElvishJerricco]

Also worth noting that withSpiderTimeline would probably perform much better without the need for the class.

[ElvishJerricco]

This compiles, but it's pretty gross. And reify should pretty much guarantee no specialization, if my understanding is correct (I have not tested that).

data ReflectedSpiderTimeline x s

instance Reifies s (SpiderTimelineEnv x) =>
         HasSpiderTimeline (ReflectedSpiderTimeline x s) where
  spiderTimeline = reflected (Proxy :: Proxy s) $ reflect (Proxy :: Proxy s)

reflected
  :: Proxy s
  -> SpiderTimelineEnv x
  -> SpiderTimelineEnv (ReflectedSpiderTimeline x s)
reflected _ = unsafeCoerce

-- | Pass a new timeline to the given function.
withSpiderTimeline :: (forall x. HasSpiderTimeline x => SpiderTimelineEnv x -> IO r) -> IO r
withSpiderTimeline k = do
  env <- unsafeNewSpiderTimelineEnv
  reify env $ \s -> k (reflected s env)

This is relevant to this PR because it influences the design of runReflexBase.

[ryantrinkle]

Hmm, I'm confused about the specialization/inlining thing, but maybe it would be a good idea to run it by some of the GHC guys. Currently, in large apps, the specialization/inlining is already pretty bad for any medium to large apps, unless you turn on -fspecialize-to-spidertimeline-global, which massacres all this stuff anyhow.

[ElvishJerricco]

My understanding of the problem in this particular case is that GHC just completely fails to do any specializing when a rank-n type has constraints.

rankn :: (forall m. MonadIO m => String -> m ()) -> IO ()
rankn f = f "hi"

unspecializable :: IO ()
unspecializable = rankn (liftIO . putStrLn)

[ryantrinkle]

@ElvishJerricco I just added your solution, with a couple of things renamed slightly.

[ElvishJerricco]

Ah cool. Question: What's the proper git-ettiquette for situations like this, where upstream makes some merge-breaking commits before merging a PR? I'm tempted to rebase this branch on the new develop, fix conflicts that way, and do a push --force, but I know forcing is kind of bad practice. That reflex-websockets example, for instance, would break (though trivially fixed).

[ryantrinkle]

Forcing is fine for a branch like this, since you don't need to keep the history and nobody else is likely to have pulled that branch.

[ElvishJerricco]

Should globalSpiderTimelineEnv and HasSpiderTimeline be exported from Reflex.Spider? I had to import Reflex.Spider.Internal to have those in scope.

EDIT: SpiderTimelineEnv as well.

[ryantrinkle]

@ElvishJerricco I've merged most of this, but not ReflexBase itself.

[ElvishJerricco]

What's the reason for leaving out ReflexBase?

[ryantrinkle]

It'd be good to get it in, but I wanted to get the other stuff in ASAP, since the other stuff is in parts of the code I'm changing rapidly. ReflexBase isn't likely to conflict with stuff even if it takes a couple of days to get in.

In order to get it in, we need to do a few things:

1. Documentation. With the exception of Reflex.Spider.Internal, I'm going to keep reflex at 100% haddock coverage going forward. I'm adding docs for your other
2. Style. I've fixed the formatting of the other code in this PR, but we need to get ReflexBase cleaned up as well.
3. Naming. I think Reflex.Base is too broad a term - to me, this monad is a bit analogous to RWST: a transformer that combines several other transformers, for convenience and (possibly at some point) performance. Also, ideally the name of the monad should not refer to Reflex by name.

One final thing: I think it would be better to move a bit more stuff into TriggerEvent. The use of Chan seems like it ought to be an implementation detail not exposed to the user. Do you have any thoughts on how to achieve that?

[ElvishJerricco]

Good points. I could have been more diligent on those three points.

Moving more into TriggerEvent touches on something that I think should precede having a ReflexBase at all, now that I think about it. I think each MonadX class should come with a concrete implementation transformer in reflex if possible, including a sensible standalone way of running that transformer with respect to the underlying monad, such as a runTriggerEventT :: ... => TriggerEventT t m a -> m a. Finally, each transformer needs to include lifted implementations of each others' class. Once all of that is in place, something like ReflexBase would be a purely superficial wrapper around all of this.

[ElvishJerricco]

I also think a more rigid convention should be followed with those classes and implementations. Specifically, each pair should have the modules Reflex.X.Class and Reflex.X.Base. Currently, a few of these kinds of modules contain more than one of these pairs, and not all of them are split into distinct modules.

[ElvishJerricco]

This is where my name ReflexBase came from. I was imagining that Reflex.Class represented the sum of all such Class modules, while Reflex.Base represented the sum of all such Base modules.

[ryantrinkle]

This morning I pushed a change that reorganized this stuff substantially. Together with the parts of this PR that I've already merged, I think "each pair should have the modules Reflex.X.Class and Reflex.X.Base" is now basically true. I agree about having clean run* functions as well, and I think most of them do, but it's not obvious how to do it in all cases, especially PerformEventT.

With regard to Reflex.Base, I see where you're coming from, and that makes sense. However, Reflex.Class is really focused on the Reflex, MonadHold, and MonadSample classes, and I'd kind of like to keep it clear of the monad transformer stuff. I think a more descriptive name would be helpful in letting people understand exactly what functionality is included or not.

[ElvishJerricco]

Ah, cool. I hadn't seen the other changes you made this morning.

So then I guess the changes still needed then are the remainder of the n^2 instances of MonadX, runX functions for whichever ones don't already have them, and some notion of the composition of each of these transformations transformers.

For the runX functions, it might be useful to adopt a continuation style for some of them. runPostBuildT, for instance, isn't very useful, since it requires you to give it the needed event. With continuations of the form runX :: ... => (m a -> n b) -> X t m a -> n b, the X monad is allowed to do some lifecycle stuff both before and after running the continuation before returning the result, such as creating an event first and firing it after in the case of PostBuildT.

[ryantrinkle]

That all sounds right to me. By the way, do you happen to have patches for reflex-dom in relation to these changes?

[ElvishJerricco]

None whatsoever. I'm guessing a couple of the monads in the Widget stack just need to have pieces replaced with the transformers now present in reflex, and the attachWidget stuff can have a lot replaced with compositions of runX functions. Or, if some ReflexBase-like thing comes along encompassing all the transformers in reflex, then reflex-dom can probably just use that as a base for Widget and attachWidget.

[ryantrinkle]

@ElvishJerricco It looks like I can't reopen this, since I merged some of it, but I created #80 to track the remainder.