-
-
Notifications
You must be signed in to change notification settings - Fork 12
Proposal: Move to a single index for hook types #32
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
Conversation
Does it still prohibit running hooks conditionally?One question I have about this. Is it possible to subvert the guarantees of this code by using regular do notation rather than the qualified do notation you are supposed to use? For example, let's say I forget to use foo = do -- notice, no Hooks.do here
state /\ stateId <- useState 0
useLifecycleEffect do
liftEffect $ log "foo"
-- Now, let's say I reorder the hooks so that the effect occurs first, does the code still compile? I imagine the indexed monad approach would not compile in this second version because the Monad instance for an indexed monad would require the "state" type to not change when using normal data IxMonad x y a = IxMonad a
-- The `state` types here don't change
instance bindIxMonad :: Bind (IxMonad x x) where
bind :: IxMonad x x a -> (a -> IxMonad x x b) -> IxMonad x x b I thought preventing this state change was one of the primary reasons for using indexed monads over something else. Otherwise, what stops you from writing -- `k` would be `HookType`, but I guess this would be easier
class IxAppendMonad :: forall k. (k -> Type -> Type) -> Constraint
class IxAppendMonad m where
iabind
:: forall a b state1 state2
. m state1 a
-> (a -> m state2 b)
-> m (state1 <> state2) b Related...
I can't recall the full context of this situation and why that was actually a "bad" thing. If you make it possible to do this, do you also make it possible to run hooks conditionally? On Naming Things
I feel like Hooks.Nil <> UseState Int <> UseEffect <> Hooks.Nil <> Hooks.Nil
foreign import data Hooked :: HookType -> HookType -> HookType
infixr 1 type Hooked as <> Could this be something more meaningful like PS 0.14 and its features
Except Just clarifying my before-and-after understandingI don't think your before-and-after examples above provided a full implementation for a real hook. They showed how to define a hook, but not the full example. Here's my current understanding based on what you've written thus far. Is this correct? -- current approach: hooks appear in reverse
newtype UseX hooks = UseX (UseState Int (UseEffect hooks))
derive instance newtypeUseX :: Newtype (UseX hooks) _
{-
useUseX :: Hooked m hooks (UseX hooks) Return -}
useUseX :: Hook m UseX Return
useUseX = Hooks.wrap Hooks.do
useLifecycleEffect do
-- do stuff here
state /\ stateId <- useState 1
pure returnValue
-- future approach: hooks appear in their written order
-- but lack of type synonyms in head requires
-- using HookEquals; limitation will be lifted
foreign import data UseX :: Hooks.HookType
instance newtypeUseX
:: HookEquals (UseEffect <> UseState Int <> Nil) h => HookNewType UseX h
useUseX :: Hook m UseX Return
useUseX = Hooks.wrap Hooks.do
useLifecycleEffect do
-- do stuff here
state /\ stateId <- useState 1
pure returnValue |
I clarified the title of this PR to note it as a proposal rather than a definite path forward. I won't merge this for a while so the idea can sit and be discussed. To answer your questions: Does this still prevent running hooks conditionally?
The code in question was this: foo = do -- notice, no Hooks.do here
state /\ stateId <- useState 0
useLifecycleEffect do
liftEffect $ log "foo"
-- The code won't compile either way without
You still need On naming things
That actually is the reason to use
That's a good point. It would be better named PS 0.14
Yes, perhaps it would be best to let this sit until 0.14, and see if it can be cleaned up even more by that time. The Clarifying before-and-after
You can also look at the -- future approach: hooks appear in their written order but lack of type synonyms in
-- instance head requires using HookEquals; limitation will be lifted
foreign import data UseX :: Hooks.HookType
instance newtypeUseX
:: HookEquals (UseEffect <> UseState Int <> Nil) h => HookNewType UseX h The code you've written here doesn't use a type synonym so you don't need foreign import data UseX :: Hooks.HookType
instance newtypeUseX :: HookNewtype UseX (UseEffect <> UseState Int <> Nil) However, if you do use a type synonym, then you need foreign import data UseX :: Hooks.HookType
type UseX' = UseEffect <> UseState Int <> Nil
instance newtypeUseX :: HookEquals UseX' h => HookNewtype UseX h |
Note, you don't have to use hook newtypes at all. You can just use synonyms everywhere if you want. Type errors can get crazy though for large blocks. |
Yea, that's true for both approaches. -- old
type UseX hooks = UseState Int (UseEffect hooks)
useX :: Hook m UseX Unit
useX = ...
-- new
type UseX = UseEffect <> UseState Int <> Nil
useX :: Hook m UseX Unit
useX = ... |
Great! That was my only real concern with this.
Ok, but we don't use
Yeah, that makes more sense.
🤦♂️ Wow... I completely forgot about those..
Looks like the advantage of this approach over the type one is that it's easier to write. However, when looking at its docs, one will need to be directed to look at its
True, but who wants to deal with crazy type errors?
Ok, so I had somewhat of an overreaction caused mainly by my fear that the code would compile using regular do notation. I think this PR's approach is better than what we currently have, and it would be more future-focused. Otherwise, we'll build an entire ecosystem based on the current approach, knowing that we'll break it all in the future anyways. Also, we likely wouldn't want to use |
Or
It also doesn't lend itself well to longer hooks -- you'd end up with weird indentation like this: instance newtypeUseX
:: HookNewtype
UseX
(UseEffect
<> UseState MyReallyLongType
<> UseRef MyReallyLongType
<> UseRef MyReallyLongType
<> Nil) |
I thought about that, but didn't want to take up the namespace in case there was a hook sometime in the future that wanted that name.
Ah... yeah. But, even the current approach suffers from this in its own way. |
I've renamed |
As a note: this can't be merged until the README and other documentation have been updated as well. |
I'm ready to merge this, though I'm still a little uncertain about using |
Other potential names:
|
@milesfrain @JordanMartinez I'm merging this PR now, but not yet making a release (I still have a few other changes that I'd like to go into the next release, and I need to write a short migration guide). However, I wanted to ping you as an early heads up on |
Thanks for the headsup! |
I’ve had some time to work with an alternate Hooks implementation which only uses a single index for hook types instead of two (as is done with the indexed free monad).
Compare:
In this approach a
Hook
is a free monad of the algebraUseHookF
andbind
appends hooks with<>
. The new approach models hooks like a list: as a series of appends.But the old approach had more going on:
Improvements
Switching to a free monad + phantom type instead of an indexed free monad makes a few improvements to how Hooks are written.
Hooks are now written in the order they occur
I find it irritating to define Hooks in the reverse order from how they appear in the Hook definition, and the same is true of type errors — Hooks get parsed backwards, so you have to flip the error in your head.
Compare:
You can now use
Hooks.pure
in place of a particular list of HooksAs @Vladciobanu noted on the functional programming Slack, it seems like you ought to be able to use
Hooks.pure
to return a Hook’s result, regardless of theHookType
of the hook.This ought to compile, but it doesn’t under the indexed monad. With the simple free monad approach this is now accepted.
Fewer type variables to understand
Hooks are now just a free monad with a phantom type variable, instead of being a full-on indexed monad. Previously, you had to leave a
hooks
variable free when defining any Hooks:Under the hood, the
Hook m hookType a
was actuallyHooked m hooks (hookType hooks) a
, which was confusing to see in error messages instead of seeing theHook
type. But all this underlying type was ever used for was to applyUnit
as thehooks
type when you turned your Hook into a component.It was easy to forget the
hooks
type and see obscure errors, or not be sure where thehooks
type would go in the nested parentheses, and so on (see JordanMartinez/learn-halogen#78 (comment) as well).Now, there is no type synonym —
Hook
is justHook
, without the indexed monad. Rather than nested parentheses you can use the<>
type operator to append hook types, where you can useNil
to represent a call topure
.If you don’t need the synonym it gets a bit smaller:
Drawbacks
The new approach does have some drawbacks, mostly in that it introduces a few concepts users wouldn’t otherwise have to learn about.
More unfamiliar concepts
foreign import data ...
will be a new concept for a lot of people in a way thatnewtype
alone is not. Luckily this is essentially boilerplate — you don’t need a deep understanding of the topic, you just need to recognize the pattern. Still, it’s yet more unfamiliar stuff to learn.HookEquals
will be new for anyone who doesn’t know aboutTypeEquals
.HookNewtype
is hopefully clear to anyone used toNewtype
, but it’s less clear than just usingNewtype
.More complicated definitions
This is how you’d define a Hook under the indexed monad — you need to know about leaving the
hooks
variable free and to nest it at the deepest level, but otherwise this is all ordinary PureScript code:This is how you’d define the same thing with the free monad + phantom type — now you need to know about the
Hooked
type /(<>)
type operator, and theHookType
kind, and theHookEquals
andHookNewtype
type classes. You need to know (or at least you’ll soon find out) that type synonyms are disallowed in instance heads, which is whyHookEquals
is needed.I don’t think any of this is hard, especially because
HookEquals
should be unnecessary soon, but it’s definitely a few new things to learn about where the newtype approach has none of them.Improvements to look forward to
The main thing I dislike about the new implementation is that users will now have to use the
HookEquals
class to get around not being able to use type synonyms in instance heads.However, I’m ultimately still open to making this change because there are upcoming changes to the PureScript compiler that will make this nicer.
First, with PolyKinds in 0.14 the
HookEquals
class is no longer necessary becauseTypeEquals
will work on theHookType
kind:Even better, if purescript/purescript#3539 gets merged for 0.14 (which I hope happens!) then the type synonym workaround via
TypeEquals
won’t even be necessary:Some example type errors
Here are a few comparisons of type errors between the two implementations. I’m happy to add more.
Used too many hooks
The HookType version does a better job of reporting that it expected a `UseEffect` hook but instead received 3:As compared to the current indexed monad, which reports it can’t match
hooks1
withUseEffect (UseEffect hooks1)
first, which is hard to parse, before a better error message that shows all the hooks. I find the newtype part of the error message easier to parse as well.Newtype differences
Any error which pertains to the use of `wrap` is better with the new version, because the old version would spit out `Hooked` out of nowhere when users would typically have never seen it before.The
wrap
function in the HookType version shows a move fromHook _ a _ -> Hook _ b _
and checks that the resulting type isHook m UseInitializer Unit
.The indexed monad version, though, is hard to understand:
Used the wrong hook
The old version was a bit better, but the
wrap
function reveals “Hooked” with extra types out of nowhere, which is worse.