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

HookM code that modifies state can't be passed between components #37

Open
marcusbuffett opened this issue May 22, 2020 · 13 comments
Open
Labels
document me Improvements or additions to documentation

Comments

@marcusbuffett
Copy link

Having an issue where calling a HookM callback from a child component results in an error like this:

Error: Failed pattern match at Data.Maybe (line 268, column 1 - line 268, column 46): Nothing2 bundled.js:2788:15
    fromJust http://127.0.0.1:4201/bundled.js:2788
    unsafeSetCell http://127.0.0.1:4201/bundled.js:39115
    interpretHalogenHook http://127.0.0.1:4201/bundled.js:39663
    interpretHalogenHook http://127.0.0.1:4201/bundled.js:39673

Here's a project that reproduces the problem: https://github.com/marcusbuffett/hooks-issue-example

This is a follow-up from messages on slack where the modal would call the close function but it wouldn't get closed, but it seems in trying to create a reproducible example, I encountered a different issue.

Thanks!

@JordanMartinez
Copy link
Contributor

This looks like it should be valid code to me. The problem is that the HookM code you define in the parent is run in the child. Thus, Halogen Hooks tries to update the corresponding state in the child's Array of states rather than the parent's Array of states, thereby producing the bug.

You can get around it by not using a separate component for the child and include that child within the parent.

parentComponent =
  Hooks.component \_ input -> Hooks.do
    recModalOpen /\ recModalOpenId <- Hooks.useState $ false
    Hooks.captures { recModalOpen } Hooks.useTickEffect do
      liftEffect $ log $ show recModalOpen
      pure Nothing
    let
      closeModal = do
        liftEffect $ log "Closing!"
        Hooks.modify_ recModalOpenId (const false)
        pure unit
    Hooks.pure
      $ HH.div
          []
          ( [ HH.div [ HE.onClick \_ -> Just (Hooks.modify_ recModalOpenId (const true)) ] [ HH.text "open" ] ]
              <> guard recModalOpen [ child closeModal ]
          )
  where
  child closeModal = 
    HH.div
          [ HE.onClick \_ -> Just closeModal ]
          [ HH.div
              []
              [ HH.text "foo"
              ]
          ]

@JordanMartinez
Copy link
Contributor

This bug should be renamed to something like, "Defining state-modifying HookM code in Component A and running it in Component B attempts to update Component B's state, not A."

@thomashoneyman
Copy link
Owner

@marcusbuffett as @JordanMartinez noticed and we briefly talked about on Slack, the problem is that it’s not safe to pass HookM code between components (though it’s perfectly fine to pass it to hooks). Once you turn hooks into a component you should start using component communication (messages and queries) instead.

I’ll need to think a little on how to make this situation either not possible or at least not cause an exception due to the unsafe internal calls to set state. Turns out they’re not hidden behind a safe interface as much as I thought.

@marcusbuffett marcusbuffett changed the title Failed pattern match at Data.Maybe , with HookM callback Defining state-modifying HookM code in Component A and running it in Component B attempts to update Component B's state, not A May 24, 2020
@marcusbuffett
Copy link
Author

Changed the title from your suggestion @JordanMartinez , and thanks for the alternate solution. I ended up passing a message up from the child component instead, as @thomashoneyman suggested on Slack. Just a bit more boilerplate but got things working now. Appreciate the help guys!

@JordanMartinez
Copy link
Contributor

I’ll need to think a little on how to make this situation either not possible or at least not cause an exception due to the unsafe internal calls to set state. Turns out they’re not hidden behind a safe interface as much as I thought.

I think it would be nice if one run code defined in one component in another component. However, I'm not sure whether the resulting implementation would conflict with the test code you would write.

If we want to produce a compiler error, would skolem types on HookM that also match the corresponding Hook type work?

@thomashoneyman
Copy link
Owner

I think it would be nice if one run code defined in one component in another component.

When you write some code in HookM it doesn't matter where you define it -- it matters where you evaluate it. HookM code will evaluate its instructions in the component where it is run, regardless of where you defined it.

That's because a component is the interpreter that runs all Hooks code. If your HookM code says to modify the state cell at index 2, then the component that runs the HookM code will do that -- even if there is no state cell at index 2.

This situation only arises when you use the state hook in one hooks-based component, then use the identifier to write some code that will be evaluated in a different hooks-based component. There's simply no way that this can work, because the identifier is only usable in the component where it was introduced. You can't pass it to another component -- it doesn't exist there.

In contrast, you can freely pass around identifiers anywhere within the component. For example, you can nest hooks a dozen deep, passing identifiers down and up the stack of hooks, and it will work just fine because they'll all ultimately be run by one component. It's only a problem once you pass one of them to a different component.

This is also the same reason why you don't pass HalogenM code among components. Instead, you use queries to tell a child component to do something or output messages to tell a parent component to do something. In ordinary Halogen no one tries to do this because of all the type parameters involved, but Hooks hide away the state type variable.

Unfortunately I don't yet have an idea of how to prevent folks from doing this without significantly affecting usability except to say "don't" in the documentation.

@thomashoneyman
Copy link
Owner

There are essentially two possible approaches here:

  1. Use a reference to the component as part of the state identifier (hidden from the user). If the state identifier is used in a component that doesn't match its reference, then crash. This makes this issue more obvious by immediately throwing an error with an informative error message, rather than fail with unsafeSetCell in the case the index doesn't exist, or modify the wrong state in the case the index does exist.

  2. Provide an interface so that you are allowed to pass HookM code from one component to another component, where the code will be executed in the original component when called by the receiving component. This would require some shenanigans via Aff to communicate between the components, incurring a MonadAff constraint on all Hooks-based components to support this use case. It's a bit of a price to pay for a situation I expect is relatively rare, though maybe it won't be as rare as I'm assuming.

@thomashoneyman thomashoneyman changed the title Defining state-modifying HookM code in Component A and running it in Component B attempts to update Component B's state, not A HookM code that modifies state can't be passed between components May 26, 2020
@MonoidMusician
Copy link

I’m wondering if something like what ST does would work here? Universally-quantified regions to encapsulate the values at the type level so they only appear in a certain scope. (I haven’t been keeping up with the discussion very well so apologies if I’m off base.)

@natefaubion
Copy link
Collaborator

It does work, but it also means all hook code must be written with a polymorphic region, with the restrictions that come with that ($ and # not working intuitively). The question is if it's worth disallowing statically with such a trade off.

@thomashoneyman
Copy link
Owner

This has partially been addressed by #44, but I would like to wait a little longer to see if a nicer solution exists before closing the issue altogether.

@JordanMartinez
Copy link
Contributor

So, how would we implement this if we weren't using Hooks and just using regular components?

For example, I once tried using HalogenM as the action type. I believe I got an "infinite type defined error" or something. So, if I newtyped HalogenM with all parameters specified for my particular component to eliminate that error, and handleAction was simply un HalogenMNewtype, could I run defined-in-component-A HalogenM code in component B? Or would component B need to use component A's query to make component A run its own code?

@thomashoneyman
Copy link
Owner

thomashoneyman commented Jun 18, 2020

@JordanMartinez You can get fancier with coercion, but this code demonstrates how you can pass a callback down from a parent to a child component; when the callback is invoked then the parent component will evaluate some code.

https://try.purescript.org/?gist=5d21035dabf1512be120dba444411b56

Expand to read the code snippet inline...
module Main where

import Prelude

import Data.Foldable (for_)
import Data.Maybe (Maybe(..))
import Data.Symbol (SProxy(..))
import Effect (Effect)
import Effect.AVar as AVar
import Effect.Aff (Aff)
import Effect.Aff.Class (class MonadAff, liftAff)
import Halogen (liftEffect)
import Halogen as H
import Halogen.Aff as HA
import Halogen.HTML as HH
import Halogen.HTML.Events as HE
import Halogen.Query.EventSource (affEventSource)
import Halogen.Query.EventSource as EventSource
import Halogen.VDom.Driver (runUI)

main :: Effect Unit
main = HA.runHalogenAff do
  body <- HA.awaitBody
  runUI parent unit body

-- Create an `Aff` function that can be used to trigger an action in one component
-- to be evaluated in another.
mkCallback :: forall st act ps o m. MonadAff m => act -> H.HalogenM st act ps o m (Maybe (Aff Unit))
mkCallback f = do
  cbAVar <- liftEffect AVar.empty

  _ <- H.subscribe $ affEventSource \emitter -> do
    let callback = EventSource.emit emitter f 
    pure mempty <* liftEffect (AVar.tryPut callback cbAVar)

  liftEffect $ AVar.tryTake cbAVar

-- The parent is able to evaluate actions triggered in the child component by
-- passing down a callback

type ParentState = { count :: Int, cb :: Aff Unit }

type Slots = ( child :: forall q. H.Slot q Void Unit )

_child = SProxy :: SProxy "child"

data ParentAction
  = Initialize 
  | Evaluate (forall o m. H.HalogenM ParentState ParentAction Slots o m Unit)

parent :: forall q i o m. MonadAff m => H.Component HH.HTML q i o m
parent =
  H.mkComponent
    { initialState: \_ -> { count: 0, cb: pure unit }
    , render
    , eval: H.mkEval $ H.defaultEval 
        { handleAction = handleAction 
        , initialize = Just Initialize
        }
    }
  where
  render state =
    HH.div_
      [ HH.p_ [ HH.text $ "You clicked " <> show state.count <> " times" ]
      , HH.slot _child unit child { cb: state.cb } absurd
      ]

  handleAction :: ParentAction -> H.HalogenM ParentState ParentAction Slots o m Unit
  handleAction = case _ of
    Initialize -> do
      -- make the callback function
      mbCallback <- mkCallback $ Evaluate do
        H.modify_ \st -> st { count = st.count + 1 }

      -- set it in state so it can be passed to child components
      for_ mbCallback \cb -> H.modify_ _ { cb = cb }
      
    Evaluate act -> act

-- The child component is able to trigger actions in its parent without sending
-- a message, by receiving a callback passed down.

type ChildInput = { cb :: Aff Unit }

data ChildAction = Run (Aff Unit) | Receive ChildInput

child :: forall q o m. MonadAff m => H.Component HH.HTML q ChildInput o m
child =
  H.mkComponent
    { initialState: identity
    , render
    , eval: H.mkEval $ H.defaultEval 
        { handleAction = handleAction 
        , receive = Just <<< Receive
        }
    }
  where
  render state =
    HH.div_
      [ HH.button
          [ HE.onClick \_ -> Just (Run state.cb) ]
          [ HH.text "Click me (child)" ]
      ]

  handleAction = case _ of 
    Receive i -> H.put i 
    Run cb -> liftAff cb

As a note, this doesn't have to be direct parent/child communication; the callback that's created here can be passed anywhere (put in global state, passed through a child component to a further child, etc.). But it requires that whatever component calls it can run in MonadAff. In contrast, using messages to accomplish the same thing doesn't incur this restriction.

@thomashoneyman thomashoneyman added the document me Improvements or additions to documentation label Aug 15, 2020
@thomashoneyman
Copy link
Owner

I think that this issue should be documented (my response here, specifically), but I don't have any plans to update the library to make this possible. If folks want it badly enough I'm willing to consider adding something, but I haven't seen it come up since #44 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document me Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

5 participants