Content-Length: 286057 | pFad | https://github.com/purescript-halogen/purescript-halogen/issues/748

14 Memoized / lazy can behave incorrectly when rendering child components · Issue #748 · purescript-halogen/purescript-halogen · GitHub
Skip to content

Memoized / lazy can behave incorrectly when rendering child components #748

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

Open
thomashoneyman opened this issue Apr 11, 2021 · 4 comments
Labels

Comments

@thomashoneyman
Copy link
Member

Summary

The lazy and memoized functions (ie. anything that uses Thunk from halogen-vdom) can cause slots to incorrectly be created and destroyed when used on HTML containing child components.

The bug appears when the unsafeThunkEq function returns true:
https://github.com/purescript-halogen/purescript-halogen-vdom/blob/ae2f2d8141223c2cc67a92282077e8045f75e921/src/Halogen/VDom/Thunk.purs#L84-L88

Notably, the bug does not appear when Halogen is rendering HTML without child components. It's only when there are child components being used with memoized / lazy.

There's something happening in the interaction between renderChild and how the VDom machines work for thunks.

Reproduction

A full reproduction of the issue, with several examples, can be found in this gist:
https://gist.github.com/thomashoneyman/0ee0ae2bd403390dc1671509c66b4a1b

which can itself be run interactively on Try PureScript:
https://try.purescript.org/?gist=0ee0ae2bd403390dc1671509c66b4a1b

In case the gist ever changes, the contents are also reproduced here:

Click to expand!

module Main where

import Prelude

import Control.Monad.Rec.Class (forever)
import Data.Maybe (Maybe(..))
import Data.Identity (Identity(..))
import Data.Symbol (SProxy(..))
import Effect (Effect)
import Effect.Aff (Aff, Milliseconds(..))
import Effect.Aff as Aff
import Effect.Aff.Class (class MonadAff)
import Effect.Exception (error)
import Halogen as H
import Halogen.Aff as HA
import Halogen.HTML as HH
import Halogen.HTML.Properties as HP
import Halogen.Query.EventSource (EventSource)
import Halogen.Query.EventSource as EventSource
import Halogen.VDom.Driver (runUI)

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

{- Each example renders a 'Count: <n>' message. The `renderCountHtml` and `renderCountSlot` functions
   below are two simple ways to render this message. The first is a plain HTML function; the second 
   uses the same function within a minimal component.
   
   This code demonstrates that when the `lazy` or `memoized` functions from Halogen are used on a slot,
   and the equality test returns `true` (ie. the renderer should not be called again, and the previous
   vdom result should be reused) that new slots are unexpectedly created. You can tell even without
   inserting debug statements into halogen-vdom because if you `spy` within the `renderCountHtml`
   function you'll see the `spy` statement print before you see the bug appear.
   
   This bug does not appear when using the plain HTML `renderCountHtml` function.
-}

renderCountHtml :: forall w i. Int -> HH.HTML w i
renderCountHtml n = HH.p_ [ HH.text $ "Count: " <> show n ]

renderCountSlot :: Int -> H.ComponentHTML Action Slots Aff
renderCountSlot n = HH.slot (SProxy :: SProxy "inner") unit counterComponent n absurd

counterComponent :: forall q o. H.Component HH.HTML q Int o Aff
counterComponent = H.mkComponent
  { initialState: identity
  , render: renderCountHtml
  , eval: H.mkEval $ H.defaultEval { handleAction = \(Identity a) -> H.put a, receive = Just <<< Identity  }
  }

-- This render function is used by all the examples, via `containerComponent`. It uses `lazy` to
-- only invoke the provided renderer if the 'count' field in state has changed. The component's
-- 'hidden' field in state is updated several times a second, but the 'count' field is only updated
-- every few seconds.
--
-- If no renderer is provided, this uses the `renderCountSlot` from module scope.
render :: State -> H.ComponentHTML Action Slots Aff
render state = 
  HH.div
    [ HP.attr (HH.AttrName "style") ("height: 300px; padding: 25px; margin: 25px; background-color: " <> state.color <> "; overflow: hidden;") ]
    [ case state.renderer of
        Just renderer -> do
          HH.lazy renderer state.count
        _ ->
          HH.lazy renderCountSlot state.count
    ]

{- Each example provides the `containerComponent` with a different renderer to use. -}

examplesComponent :: forall q i o. H.Component HH.HTML q i o Aff
examplesComponent = H.mkComponent
  { initialState: identity
  , eval: H.mkEval H.defaultEval
  , render: \_ -> HH.div
      [ HP.attr (HH.AttrName "style") "display: grid; grid-template-columns: 1fr 1fr; gap: 25px" ]
      [ -- This container is passed a simple `Int -> HH.HTML w i` function to render the count.
        -- This renderer behaves as expected: when the 'hidden' count updates, the `lazy` refEq
        -- test for the 'count' field returns `true`, and the count is not re-rendered.
        container 0 { color: "cadetblue", renderer: Just renderCountHtml }
        
        -- This container is passed a slot containing the `counter` component. This renderer does
        -- not behave as expected. When the `lazy` refEq test returns `true`, a new slot is created.
      , container 1 { color: "antiquewhite", renderer: Just renderCountSlot }
      
        -- This container is passed a slot containing the `counter` component, wrapped in a div.
        -- This renderer does not behave as expected. When the `lazy` refEq test returns `true`,
        -- a new slot is created and immediately destroyed.
      , container 2 { color: "darkseagreen", renderer: Just \n -> HH.div_ [ renderCountSlot n ] }
      
        -- This container uses the `renderCountSlot` function from module scope instead of accepting
        -- it as an argument. This renderer also doesn't behave as expected.
      , container 3 { color: "lightgray", renderer: Nothing }
      ]
  }
  where
  container id input = HH.slot (SProxy :: SProxy "container") id containerComponent input absurd

{- The 'container' component updates the 'hidden' counter multiple times a second and the 'count'
   counter once every few seconds.
  
   The 'container' component guards the 'Count: <n>' renderer behind a call to `HH.lazy`. The count
   should therefore only re-render every few seconds (when the 'count' field is updated), but it
   should not re-render when the 'hidden' count updates.
-}

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

type Input =
  { color :: String
  , renderer :: Maybe (Int -> H.ComponentHTML Action Slots Aff)
  }

type State = 
  { color :: String
  , renderer :: Maybe (Int -> H.ComponentHTML Action Slots Aff)
  , count :: Int
  , hidden :: Int
  }

data Action = Initialize | TickHidden | TickCount

containerComponent :: forall q o. H.Component HH.HTML q Input o Aff
containerComponent =
  H.mkComponent
    { initialState
    , render
    , eval: H.mkEval $ H.defaultEval { handleAction = handleAction, initialize = Just Initialize }
    }
  where
  initialState :: Input -> State
  initialState { color, renderer } =  { color, renderer, count: 0, hidden: 0 }
    
  handleAction :: Action -> H.HalogenM State Action Slots o Aff Unit
  handleAction = case _ of
    Initialize -> do
      _ <- H.subscribe $ timer (Milliseconds 400.0) TickHidden
      _ <- H.subscribe $ timer (Milliseconds 2000.0) TickCount
      pure unit

    TickCount ->
      H.modify_ \state -> state { count = state.count + 1 }
  
    TickHidden ->
      H.modify_ \state -> state { hidden = state.hidden + 1 }

timer :: forall act m. MonadAff m => Milliseconds -> act -> EventSource m act
timer ms act = EventSource.affEventSource \emitter -> do
  fiber <- Aff.forkAff $ forever $ Aff.delay ms *> EventSource.emit emitter act
  pure $ EventSource.Finalizer $ Aff.killFiber (error "Event source finalized") fiber

The example contains plenty of comments describing what I would expect to see, and places to hook in debug statements. The example is implemented for Halogen 5, only because that's what Try PureScript currently expects, but the issue is also present in Halogen 6.

@garyb
Copy link
Member

garyb commented Apr 11, 2021

I think this might be a bit tricky to fix, I have a feeling it's to do with the children being popped / reinserted by renderChild, but I'd have to dig into it a bit more for sure.

One possible kludge for now would be to restrict the type of the thunked rendering to only accept HTML with () slots, so at least people can't trigger this on purpose using the functions we provide.

@thomashoneyman
Copy link
Member Author

From @natefaubion -- the main Halogen render needs the renderChild function to be called for all child components, because that's how it keeps track of which components were removed / created / etc. The renderChild function takes it out of one Map and puts it in another. When you introduce lazy / memoize, the VDom thunk machine stops, and it doesn't do any further diffing or processing. If that thunk has child slots then renderChild won't be called for them, so Halogen thinks the component lifecycle is done, and so it removes the component from its internal state -- but the component is still in the DOM.

These snippets show the evaluation, in order:

https://github.com/purescript-halogen/purescript-halogen/blob/master/src/Halogen/Aff/Driver.purs#L176-L181
https://github.com/purescript-halogen/purescript-halogen/blob/master/src/Halogen/Aff/Driver.purs#L208-L227
https://github.com/purescript-halogen/purescript-halogen/blob/master/src/Halogen/Aff/Driver.purs#L182-L187
https://github.com/purescript-halogen/purescript-halogen/blob/master/src/Halogen/VDom/Driver.purs#L83-L90
https://github.com/purescript-halogen/purescript-halogen/blob/master/src/Halogen/VDom/Driver.purs#L115-L116

The fix isn't easy; it would require re-architecting things so that Halogen's render doesn't rely on renderChild performing effects as part of a vdom diff for tracking lifecycle state.

@thomashoneyman
Copy link
Member Author

thomashoneyman commented Apr 13, 2021

It does seem sensible to restrict the functions to only accept HTML without child components given that the behavior is unexpected and undesirable otherwise (in lieu of an actual fix).

@natefaubion
Copy link
Collaborator

I think if you wanted to fix this you would rely on the machine finalizer (destroy) to track pending removals. The current arch works a bit like a moving GC. Have 2 heaps (old and new), and as we visit nodes we move live nodes from the old to the new heap. At the end of this process, everything left in the old heap can be removed. Thunking creates problems because live nodes may not get visited, and thus are incorrectly collected.

The tricky bit is that because slots are scoped over a component, Halogen actually lets you relocate a component (DOM identity and all) between DOM parents. That's to say, keys lets you maintain identity within a DOM parent, and slots maintain identity within a component parent. VDOM diffing only knows about DOM parents, so you cannot naively finalize a component slot when it's VDOM node is destroyed.

For example, say you have DOM children A and B. Slot C is a child of B, but we move it to A. The VDOM diffing algorithm visits A first and sees C is new and needs to be added to A. The Halogen component integration sees that this is an existing slot, and moves the DOM node to the new parent. The VDOM diff then visits B and sees C needs to be removed from B. But again, the Halogen component integration must see that this slot is still live in A, was moved, and nothing needs to be done.

Currently we assume that not visiting a node means the slot is dead, but with thunking that's not true. We should assume that the slot is still alive, and we should only potentially consider a slot dead when it's VDOM finalizer is invoked. We then must wait until the patch is completed, and we can see if indeed that node is actually dead and we should run component finalizers, or it was just moved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/purescript-halogen/purescript-halogen/issues/748

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy