Missing `onremove` event
Closed this issue · 16 comments
I noticed that hyperapp's onremove events don't work on the nestable element itself and also its children. I was wondering why that is?
Hi sorry for the late reply. Thanks for bringing it to my attention! I'm not sure if there is any (good) reason or if it's a bug. I'll look into it!
Ok it's coming back to me now... (haven't used this lib in a while 😄 )
So, first of all, hyperapp's lifecycle events are meant to be used on vdom-nodes, not components. So, in general, this doesn't work in hyperapp: <SomeComponent onremove={(el, done) => ... } /> It would only work if SomeComponent is implemented in such a way as to pass the value of the onremove-prop on to the vnode it returns.
Nestable components use the lifecycle-events on the wrapping element to do other stuff. There's the uninit prop for doing things whenever the element is destroyed.
Which brings me to another question: are you sure it's onremove you want, and not ondestroy?
onremove is a quite special one. It's only called if it is on the top element removed. If the element with onremove set gets removed as the result of its parent being removed, it is not called. Also, it prevents the element from being actually removed until you call the done function (passed as second arg to onremove handler). The only use case for this event handler, as far as I know, is to make elements leave the page in some animated way (fade-out, slide-out, et c)
If what you really need is to do some cleanup when the nestable component is removed, use the uninit prop. That binds to the ondestroy handler of the element, which is called whenever the element is removed, regardless of wether it was itself or a parent that got removed.
However, with all that said, I don't know of any reason why both ondestroy or onremove shouldn't work on children of a nestable component. Can you share a codepen/jsfiddle with a small example? Perhaps there is some mistake I can spot.
Hey @zaceno
Thanks for the very comprehensive explanation! I am working on a web app, Flamous Music (GitHub repo here), and my use case for the onremove event is indeed a slideout animation of a page before it gets destroyed.
onremove is a quite special one. It's only called if it is on the top element removed.
Now it makes sense! I initially tried onremove events within children of the nestable component, but since onremove events are only called on the top level removed element, it was not called on them, because I remove the whole nestable component (parent)
Ideally, I could just navigate to a new location using hyperapp-router's Link component and then the page slides out before it gets destroyed.
Now, my app can also work without the onremove event, as I am also in control of the routing and my code could slide the page out and defer changing the location path until the animation finishes. I am partially doing this already, but using the onremove event would make things a bit simpler.
The code in Question looks something like the following (Page.js, from line 275):
const Page = nestable(
{
...
},
{
...,
slideOut: (element, done) => {
// Do slide out
done()
}
},
(state, actions) => (props, children) => {
return <StyledPage
{...props}
class='page'
key={props.key}
oncreate={!props.hasOwnProperty('nonInteractive') && actions.makeInteractive}
onremove={actions.slideOut} // Will not be called because its parent is being removed ('flamous-page')
>
<div>
{children}
</div>
</StyledPage>
},
'flamous-page')
export default () => <Page onremove={(element, done) => { // Never called because not supported. On a normal Hyperapp component slide-out would be done here }} />In the code above the onremove function handler in the last line never gets fired.
The Page component above coresonds to a page on flamous.io . You can see it in action when you click e.g. on the About link on flamous.io
Ah, so here's the mistake:
On a normal Hyperapp component slide-out would be done here
It would not, because lifecycle events only work on vdom nodes, i e nodes that represent actual html elements. Components, on the other hand, are simply functions that take props and children as arguments – nothing more magical about them. They usually return a vnode, but not necessarily! It could be an array of vnodes, or it could be a plain string (or even null/false).
So if you want to be able to use lifecycle events on a component, the component needs to implement special support for that, by passing them along from the props to the vnode it returns. E g:
const MyComponent = (props, children) => (
<div class={"special " + (props.selected ? "highlight" : "")} onremove={props.onremove}>
{children}
</div>
)However, you are correct that Page is no normal Hyperapp component -- since it's a nestable, it handles the creation of the wrapping DOM node for you, and doesn't support passing the onremove handler on like the regular component example above.
Now that I understand what you're doing a little better, I think you have two options:
(Side note: I don't think you can have your slide-out as an action, because you won't get the second argument. Just have it as it's own function.)
a) The easy way: define your Page-component wrapped in an extra div:
const Page = nestable(...)
const slideOut = (el, done) => ...
export default () => <div onremove={slideOut}><Page /></div>b) the harder but perhaps cleaner way (depending on your p o v): fork nestable, and make it so that the wrapping element, in onremove, calls the uninit function if it's there, then calls the passed in onremove function (if it's there). That should be relatively easy but I'm not set up with a test case to try it out on, and I don't want to code in the dark, so I'll let you try it out :) If it works, feel free to submit a PR!
Actually I will give coding in the dark a shot. See if this works for you:
import {h, app} from 'hyperapp'
export default
function nestable (state, actions, view, tagname) {
actions._$r = function () {return {}}
return function (props, children) {
return h(tagname || 'x-', {
key: props.key,
id: props.id,
class: props.class,
oncreate: function (el) {
var wired = app(
state,
actions,
function (s, a) {
var v = view(s, a)
if (typeof v === 'function') v = v(el._$p, el._$c)
return v
},
el
)
el._$p = props
el._$c = children
el._$r = wired._$r
el._$u = wired.uninit
wired.init && wired.init(props)
props.oncreate && props.oncreate(el)
},
onupdate: function (el) {
el._$p = props
el._$c = children
el._$r()
props.onupdate && props.onupdate(el)
},
ondestroy: function (el) {
el._$u && el._$u()
},
//THIS IS THE NEW PART:
onremove: function (el, done) {
if (!props.onremove) return done()
props.onremove(el, done)
}
})
}
}(forget what I said about the uninit. It doesn't matter to you, and I think it's called anyway via the ondestroy lifecycle event)
If that works, let me know!
Ok, I played around with it a bit and it works (although with a slight modification)! and thanks for the code, it worked really well.
But first things first:
a) The easy way: define your Page-component wrapped in an extra div
Ok, so that does not work for me, because of what I use nestable for: I use the state to store an animation object, that has a reference to the DOM element. The animation object contains all sorts of things like current speed, current position and so on. I do that so I can control the animation from actions (they get the state, which means access to the animation object).
const Page = nestable(...)
const slideOut = (el, done) => { /* Doesn't have access to Page's state */ }
export default () => <div onremove={slideOut}><Page /></div>So if I would wrap the nestable in a div and put the onremove handler on that one, I would not have access to the animation object, which I need.
Now to option b):
The code you provided works perfectly! There is only one more one thing I needed. Consider this code:
const Page = nestable(...)
export default (props, children) => <Page onremove={(elem, done) => { /* Doesn't have aces to wired actions */ }}>{children}</Page>In order to overcome the missing wired actions I added them to the nestable DOM element in the oncreate handler:
...
el._$r = wired._$r
el._$u = wired.uninit
el.actions = wired // <-- THIS (could also be named like `el._$w` or `el._$a`)
wired.init && wired.init(props)
props.oncreate && props.oncreate(el)
...So now I can do (this is the actual code I used testing this):
export default (props, children) => <Page {...props} onremove={(elem, done) => elem.actions.slideOut(done)}>{children}</Page>
I call my wired slideOut action and give it the done function that is provided by the onremove event handler. slideOut has access to the animation object and can call done when the animation is finished.
FYI: I don't handle the animation stuff myself. For that I use Popmotion 🔥
I am using nestable from hyperapp-context, so I created 2 seperate PRs. ⬆️
What do you think about having the wired actions on the nestable DOM node?
The only thing I am not sure about is, if the name el.actions is appropriate, because it is rather general. Maybe el._$w or el._$a (similar to the others) would be better?
Before:
After:
@christiankaindl Hi thanks for the PR's!
Sorry I've been so slow to check back in. I've been really busy lately.
I'm not really sure about adding the actions to the element. If the only reason you need them is to get to your "slideOut" action, then I'm still not convinced it needs to be an action in the first place (but I haven't checked your use case out closely enough) Either way, if you really do want them added to the element, you can do it yourself via oncreate.
<MyNestableComponent oncreate={el => el.actions = actions} .... />...so I don't really think it needs to be a part of nestable
Anyhow it's a separate issue, so even if we do add it, it should go in a separate PR.
Please remove the commit about adding actions to the elements. Then I will happily merge your PRs and release new versions of nestable and context
Thanks again!
Either way, if you really do want them added to the element, you can do it yourself via oncreate.
That is interesting, but I don't think it would work for me. See, the slideOut function would need to have access to the state of the nestable and the only way I see that possible is to add that function as an action to the nestable:
const Page = nestable(
{
// state for the animation and other stuff
},
{
....,
slideOut: () => (state, actions) => {...} // <-- I would need to call this from the onremove
},
() => {
// return view function
})I don't know where actions in your snippet would come from, but I would guess it does not have access to the state (a.k.a. is not wired to the nestable)
If there is any other way to add the wired actions to Page DOM element (or for that purpose be able to call Pages wired actions in any other way) I would happily do that instead of proposing it for Nestable.
Anyway, exposing a nestable's actions in some way is a completely different issue than what I asked for in the first place, so I'll open a seperate issue for that.
Please remove the commit about adding actions to the elements. Then I will happily merge your PRs and release new versions of nestable and context
Ok, I'll do that. I am not at home for the next few days so that is probably delayed until monday, but I'll gladly do that! Thanks a lot.
Oh ok, I see hmm... yes do open another issue to discuss adding the nestables actions to the element.
I'm not opposed to it, by any means. But since there is really only one use case I can think of, I'm hopeful we can find a way to solve it in user land..
Speaking of, how do you feel about:
//nestable's view
(state, actions) => (
<div oncreate={el => el.parentNode.actions = actions}>
...
</div>
)If it works I am all in :D
I'll try it out in a minute, thanks. Did not think about that 👍
I removed the 'Add actions to nestable element' commit from both Pull Requests! So they should be ready for you
@zaceno Tested it out locally and your solution works for me 🔥
//nestable's view
(state, actions) => (
<div oncreate={el => el.parentNode.actions = actions}>
...
</div>
)Thanks so much!
I created a seperate issue for "How to make actions available in lifecycle events?" in #8, for further discussion and for reference, so that people in the future can find it better.
Thanks again @zaceno, your support was awesome!
Closing this now :)