Closed purefunctor closed 1 year ago
Nice! Before I dive into the review, it would be good to stay high level for a bit and discuss benefits and potential drawbacks.
The performance benchmarks are roughly the same, so there's no benefit or drawback there.
The type signature for a component is still the same in terms of the number of constraints and type variables, so there also seems to be no benefit or drawback, although you mentioned that we may be able to tighten up the type signature?
What do you see, in general, as the biggest benefits of the PR? Could you provide before/after examples (like if there is better inference, where inference failed before but succeeds here)?
Off the top of my head, things that I think this line of work could improve are:
fromEvent
I'm not sure if the PR does these things yet, but those seem in the ballpark of what it could do.
If it provides other benefits, make sure to list those as well! Ditto for future benefits that it doesn't provide yet but lays the groundwork for.
Thanks again for the PR! My role as maintainer is to ask the tricky questions, but regardless of how this PR pans out, I'm super impressed by your ability to dive into the code and wrangle it like this!
Going forward, what this PR entails is that it makes the intent of rendering on the browser versus server-side plus hydration by reflecting it in the type of the Domable
being built by the user. More specifically, Domable Effect
is for the latter; Domable (ST s)
is for the latter; and Domable (Mermaid s)
allows effects to be elided during server-side rendering, but can be restored during hydration, which removes the need for Always
-based coercions. Finally, a MonadST s m => Domable m
is a component that can fit within any context.
This PR also eliminates the need for the Korok
type class and the Always
constraints that it requires; the DOM functions should now infer to:
app :: forall s m l p. MonadST s m => Domable m l p
which is a lot more manageable in terms of mental overhead. However, one such downside of this design is that the runInX
functions have to accept monomorphic Domable
types and cannot be called inline:
main = runInBody (text_ "Hello, world.") -- oh no!
But, we could always provide a Prelude
that provides monomorphized versions of the top-level functions meant to be consumed by those just starting out. Alternatively, we could provide coercion functions at the top-level.
inEf :: forall l p. (forall m. MonadST Global m => Domable m l p) -> Domable Effect l p
inEf = unsafeCoerce
main :: Effect Unit
main = runInBody (inEf (text_ "Hello world"))
Going over to some implementation details however, the MonadST
constraint required by DOM functions seems to be as minimal as we can have it, since Bolson
itself requires MonadST
with flatten
. While it'd be nice to have no constraints at all, this is still an improvement over Korok
.
Going forward, what this PR entails is that it makes the intent of rendering on the browser versus server-side plus hydration by reflecting it in the type of the
Domable
being built by the user. More specifically,Domable Effect
is for the latter;Domable (ST s)
is for the latter; andDomable (Mermaid s)
allows effects to be elided during server-side rendering, but can be restored during hydration, which removes the need forAlways
-based coercions. Finally, aMonadST s m => Domable m
is a component that can fit within any context.
Is there now the possibility to make all of Deku monomorphic to Domable (Mermaid Global)
and then make the ssr/effect/hydration distinction via the interpreter? This would be a strong argument for the PR: it would simplify type signatures and likely improve errors.
For a newcomer to the project, I'm not sure that app :: forall s m l p. MonadST s m => Domable m l p
has less mental overhead than app :: forall s m l p. Korok s m => Domable m l p
. If they were familiar with ST
then it would be easier to grok what the former is doing, but that doesn't seem like a strong benefit (yet).
Is there now the possibility to make all of Deku monomorphic to
Domable (Mermaid Global)
and then make the ssr/effect/hydration distinction via the interpreter? This would be a strong argument for the PR: it would simplify type signatures and likely improve errors.
It would lead to the least amount of friction, at least in terms of better UX and having a simpler abstraction to elide effects as needed. Although, it also means that all existing Domable Effect
apps should convert their AnEvent Effect
using fromEfEvent
as provided by the mermaid
library. Similarly, apps that are built for SSR/Hydration would have to go through a similar process with replacing fromEvent
calls with fromEfEvent
.
For a newcomer to the project, I'm not sure that
app :: forall s m l p. MonadST s m => Domable m l p
has less mental overhead thanapp :: forall s m l p. Korok s m => Domable m l p
. If they were familiar withST
then it would be easier to grok what the former is doing, but that doesn't seem like a strong benefit (yet).
I agree. FRP in itself is a pretty difficult topic, but one edge that Deku has with FRP+SDOM is that it's easy to build apps that just work as opposed to the more boilerplate-y approach that (Hooks-less) Halogen takes. Is hiding the m
ultimately restrictive to the user? For what it's worth, it seems like it's only either Effect
and ST
that's used for it.
I ran some benchmarks to compare current main
with Domable (Mermaid Global)
-based components on the todo
test and got these metrics:
{
"hookChange": {
"scriptTime": "-1%",
"peakHeap": "2%",
"averageHeap": "-2%",
"averageFPS": "9%"
},
"dekuChange": {
"scriptTime": "2%",
"peakHeap": "0%",
"averageHeap": "1%",
"averageFPS": "-4%"
},
"componentChange": {
"scriptTime": "12%",
"peakHeap": "6%",
"averageHeap": "2%",
"averageFPS": "-2%"
}
}
Is there now the possibility to make all of Deku monomorphic to
Domable (Mermaid Global)
and then make the ssr/effect/hydration distinction via the interpreter? This would be a strong argument for the PR: it would simplify type signatures and likely improve errors.It would lead to the least amount of friction, at least in terms of better UX and having a simpler abstraction to elide effects as needed. Although, it also means that all existing
Domable Effect
apps should convert theirAnEvent Effect
usingfromEfEvent
as provided by themermaid
library. Similarly, apps that are built for SSR/Hydration would have to go through a similar process with replacingfromEvent
calls withfromEfEvent
.For a newcomer to the project, I'm not sure that
app :: forall s m l p. MonadST s m => Domable m l p
has less mental overhead thanapp :: forall s m l p. Korok s m => Domable m l p
. If they were familiar withST
then it would be easier to grok what the former is doing, but that doesn't seem like a strong benefit (yet).I agree. FRP in itself is a pretty difficult topic, but one edge that Deku has with FRP+SDOM is that it's easy to build apps that just work as opposed to the more boilerplate-y approach that (Hooks-less) Halogen takes. Is hiding the
m
ultimately restrictive to the user? For what it's worth, it seems like it's only eitherEffect
andST
that's used for it.
I think this is fine, we can do a tour of the community and see what apps would be hit the hardest. Most are polymorphic over m
.
I'd recommend continuing the PR by making everything monomorphic in Mermaid - once the basics are up, I can help migrate the examples.
From there, we could even be able to newtype Domable and sweep all of the type variables under the carpet, that'd be so nice...
vbussed
is still broken, presumably because of how it's implemented through FFI 😄
vbussed
should now work with the latest changes introduced in hyrule/pure/zora
.
This PR changes the top-level functions and interpreters to work on a polymorphic
m
, which is either unlifted intoEffect
orST
.Summary
Domable
s through the element functions now just requireMonadST
, which can be further elided;Toplevel
functions andInterpret
interpreters make use of theUnliftEffect
andUnliftST
type classes, allowing them to accept polymorphicDomable
sDomable
s have to have a monomorphicm
before they're passed to therunX
functions, making it impossible to write expressions such asrunInBody (C.text_ "Hello, World")
Mermaid
, there's a bug in hydration where it fails to find a node to set the text to, presumably because of how during SSR, no placeholder element was inserted as a marker.