Hyperapp: Animations, Transitions & Experiential Sites

[mattdesl]

Hey there! I'm really enjoying Hyperapp so far. I'm trying to use it for building a rich/experiential/animated site (imagine lots of animations/transitions, WebGL/Canvas, etc) and it seems to have a couple of unique issues.

1. When using onremove with a delayed callback (e.g. fade out), if the user quickly adds/removes the component from the tree they will end up with multiple of the same component. I am hoping for something closer to "React Transition Group" so that my view looks like this. But in this case I don't want multiple error messages to stack up.

<div>
  <TextField />
  <TransitionGroup>
    {state.isError ? <AnimatedErrorMessage /> : undefined}
  </TransitionGroup>
</div>

1. Right now, using onremove sometimes leads to an error:

Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

Full code to reproduce. Try clicking +/- really quickly.

import { h, app } from 'hyperapp';

const noop = () => {};

const state = {
  count: 0
};

const actions = {
  down: () => state => ({ count: state.count - 1 }),
  up: () => state => ({ count: state.count + 1 })
};

const fadeIn = (el) => {
  console.log('fade in', el);
};

const fadeOut = (el, done = noop) => {
  console.log('fade out', el);
  setTimeout(() => done(), 1000);
};

const animations = {
  oncreate: fadeIn,
  onremove: fadeOut
};

const Message = (state, children) => {
  return <div {...state} {...animations}>Above one!</div>;
};

const view = (state, actions) => {
  return <main>
    <h1>{state.count}</h1>
    <button onclick={actions.down}>-</button>
    <button onclick={actions.up}>+</button>
    {state.count >= 1 && <Message key='message' />}
  </main>;
};

app(state, actions, view, document.body);

1. This is more a question... I often end up needing local state in a component. I have seen a few issues mentioning this but they all point to different things: components, modules, mixins, embed utility functions that call app(), etc. I'm wondering what the official/suggested solution is for this?

For example: storing Canvas2D context for a component, or storing the millisecond time that the component was _first_ created (but not replacing it on subsequent renders). I do not want to move all of this information into my global app state/actions.

[zaceno]

1) What you're asking for sounds close to https://github.com/zaceno/hyperapp-transitions

2) Try:

const Message = (state, children) => {
  return <div key={'msg-' + state.count} {...state} {...animations}>Above one!</div>;
};

3)
The official answer is "no", no local state. Assemble all your state/actions in the single store, and pass it down the tree of purely functional components. Myself, I agree with you, and use stateful components occasionally.

"embed untility function that calls app()" sounds like a pattern I put forth a while back, and one I still use. IIRC the one I called "embed" used the now non-existent feature called "custom events". Today I use this instead: https://github.com/zaceno/hyperapp-nestable/ -- it's the same basic idea of calling app in the oncreate lifecycle event -- but this one works with hyperapp 1.0

"mixins" and "modules" are outdated concepts btw.

[mattdesl]

Thanks for the answers! I tested #2 as you suggest, but still see the same error when clicking the buttons quickly.

I'll investigate those libraries and see how it goes.

[zaceno]

@mattdesl Hmm strange... I tried it on codepen, and couldn't see any errors.
https://codepen.io/zaceno/pen/mpoYWQ?editors=0010

I noticed a thing (and this is probably not at all relevant, but anyway): The key "messasge" in <Message key='message' /> will not have any effect, because you're only passing the prop {key: "message"} to your Message(props) function -- but the implementation of that function isn't using the prop.

See also this issue: https://github.com/hyperapp/hyperapp/issues/555

[mattdesl]

I can reproduce it on that codepen – you have to hit +, then -, then +, then -, and repeat really quickly (so that it keeps going down to 0).

Also, doesn't the following mean that it will take the props passed from above, so it should include they key?

const Message = (props, children) => {
  return <div {...props} {...animations}>Above one!</div>;
};

[zaceno]

@mattdesl you're right about {...props} -- that's does propagate the key. So it makes sense my suggestion earlier won't make a difference.

I was able to replicate it too now. But I'm honestly not sure why it's happening. ... 🤔

[mattdesl]

Hmm, seems like when multiple <divs> co-exist (while waiting for their fade out to complete), the patch() function associates the wrong DOM element with its virtual node.

e.g. It loops over oldNode.children, which only has one div, but meanwhile element.childNodes has _two_ divs. The old element is the last one in the element.childNodes array, so it ends up getting ignored.

Then the 2 "done" callbacks trigger in series: the first removes the correct node, but the second removes the incorrect (already removed) node. This ends up giving you the error.

[jorgebucaran]

@mattdesl @zaceno I'll have a look at this later, but it sounds similar to https://github.com/picodom/picodom/issues/68.

[zaceno]

That makes sense. At least a part of the solution ought to be not using the same key ("message") on all the Message components. Because the key is how the patch knows which elements belong to which vnode. So I imagine, somehow, using the same key confuses patch about which element should be removed.

...only that's probably not the whole story. Since the key-element connection is only relevant for nodes in the vdom -- i.e those that haven't yet been removed.

I'm wondering if the patch is somehow getting confused when it adds a new "Above one!" message where it expects there to be no elements, but there are elements because they haven't been removed yet.

[jorgebucaran]

We need a test case for this.

[zaceno]

@JorgeBucaran I seem to recall a picodom issue similar to this one, but I don't think it was the one you linked to. Because that one is mostly (IIRC) about how the removed nodes get shifted to the bottom (out of range of the patch).

...actually maybe it is related... 🤔

[zaceno]

I'm wondering if the patch is somehow getting confused when it adds a new "Above one!" message where it expects there to be no elements, but there are elements because they haven't been removed yet.

@JorgeBucaran this might make an interesting test case. I suspect this might be how we reproduce the bug,

[jorgebucaran]

@zaceno What if check if element is a child of parent here:

https://github.com/hyperapp/hyperapp/blob/27c121e3b4ef8461baeb2500523217cd4289db6d/src/index.js#L209-L211

[zaceno]

@JorgeBucaran That would prevent the particular error in @mattdesl 's example. But I worry that would only lead to some other error elsewhere. Maybe...

Anyhow, It seems there's some kind of scenario we don't anticipate happening. When would element not be a child of parent in done? Answer: If some other done already removed it.
=> Meaning there are two onremove being called with the same element.
The fact that that can happen worries me. 😅

[jorgebucaran]

If some other done already removed it, then it's gone, so we could safely skip calling removeElement.

[zaceno]

@JorgeBucaran If some other done already removed it, then it's gone, so we could safely skip calling removeElement.

True. It would solve the problem and prevent the error. But how are we getting two onremove being called with the same element? I feel like unless we understand how that happens, there's still a monster under the bed.

[jorgebucaran]

Hmm. 🤔

We are calling removeElement in three different places, maybe the bug is near there.

[mattdesl]

Even if you fix the error message by checking the parent element, it will not solve the more problematic issue: that the virtual node is not associated with the correct element.

If you give each element a unique key, the problem becomes a bit more clear:
https://gist.github.com/mattdesl/fc05bf3ada731019ea49eba796c9fe68

Add a conditional breakpoint here (specifically for my above test case) and the debugger will break when the error happens. To reproduce: quickly push +, then push -, then push + again.

In the broken state, you see that the oldElement does not match the virtual oldChild.

Perhaps the problem is further than that, but for sure there is something funky going on in patch(), and the removeElement stuff is just a side effect of a larger issue.

[jorgebucaran]

@mattdesl And to clarify, you were using keys correctly? What is data-key?

[mattdesl]

From what I can tell this issue happens regardless of how you set up your keys.

[jorgebucaran]

Thanks for clarifying that. Do you think you'd be able to create a failing test case?

[mattdesl]

Yup, I'll try. :smile:

[mattdesl]

Here is a test case, it's a bit wonky since I had to use setTimeout. You'll see how it throws an error right now, but then if you replace removeElement with a safety check, the error will be gone but the test will fail as the elements won't be removed.

test('removes all tags', done => {
  const state = {
    count: 0
  }

  const actions = {
    finish: () => () => {
      expect(document.body.innerHTML).toBe(
        `<div></div>`
      )
      return done()
    },
    up: () => state => ({ count: state.count + 1 }),
    down: () => state => ({ count: state.count - 1 })
  }

  const animations = {
    onremove: (el, done) => {
      setTimeout(done, 100)
    }
  }

  const view = (state, actions) => (
    h('div', {}, [
      state.count > 0 && h('span', {
        onremove: animations.onremove,
        key: 'one'
      }, [ 'One' ]),
      state.count > 0 && h('span', {
        onremove: animations.onremove,
        key: 'two'
      }, [ 'Two' ])
    ].filter(Boolean))
  )

  const main = app(state, actions, view, document.body)
  main.up();
  setTimeout(() => {
    main.down()
    setTimeout(() => {
      main.up()
      setTimeout(() => {
        main.down()
        setTimeout(() => main.finish(), 120);
      })
    })
  })
})

[jorgebucaran]

@zaceno Do you know if this is related to https://github.com/jorgebucaran/ultradom/issues/68?

[jorgebucaran]

I had a look at the problem with the test case above and was able to confirm it's the same as https://github.com/jorgebucaran/ultradom/issues/68.

For starters we could avoid the runtime error by adding:

if (element.parentNode === parent) {
  parent.removeChild(removeChildren(element, node))
}

...but that does not solve the actual problem.

---

I think we may be able to solve this when I rewrite the diff algorithm in #499 and begin keeping a reference to the DOM element in the virtual node.

[jasoncslaughter]

I am wondering if there is any progress for fixing this issue, or at least a way of working around it?

[jorgebucaran]

Hi @rexrhino! I assume you mean the issue with deferred removals and animation. #663 helps a bit with this and Hyperapp 1.2.6 was published with this fix just the other day (no official release notes until the next minor or major version, though).

Now, the problem has not been completely resolved, but I already know the solution. Basically, we need to start keeping a reference to the element in the virtual node object, so that when onremove is called, there is absolutely no chance the data has changed (e.g., no element). This is one of the things I want to work on after 2.0, time permitting.

I'm closing here in favor of https://github.com/hyperapp/hyperapp/issues/499, which boils down to the same thing.

---

[infinnie]

663 does not solve that problem. Not even close. It just happens to appear to solve it under certain circumstances.

[jorgebucaran]

663 helps a bit, that's all I said. A definitive fix requires #499.