-
Notifications
You must be signed in to change notification settings - Fork 219
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
Comments
I think this might be a bit tricky to fix, I have a feeling it's to do with the children being One possible kludge for now would be to restrict the type of the thunked rendering to only accept HTML with |
From @natefaubion -- the main Halogen render needs the These snippets show the evaluation, in order: https://github.com/purescript-halogen/purescript-halogen/blob/master/src/Halogen/Aff/Driver.purs#L176-L181 The fix isn't easy; it would require re-architecting things so that Halogen's render doesn't rely on |
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). |
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. |
Summary
The
lazy
andmemoized
functions (ie. anything that usesThunk
fromhalogen-vdom
) can cause slots to incorrectly be created and destroyed when used on HTML containing child components.The bug appears when the
unsafeThunkEq
function returnstrue
: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!
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.
The text was updated successfully, but these errors were encountered: