Component retains old event listener after its content changes

[v4n]

I have the following component with a span that should only call unclick when it's clicked but calls click and unclick.

function component() {
  if (...) {
    return <div>
      <div></div>
      <div onclick={click} class="click-me">
        Click me
      </div
    </div>
  }

  return <div>
    <div></div>
    <div class="unclick-me">
      <span onclick={unclick}>Unclick</span>
    </div>
  </div>
}

Here is a running example:
https://codepen.io/v4n/pen/bgQNGW?editors=0010

[tivac]

Mithril redraws after the unclick function runs because of the m.mount, and it seems like that happens faster than the event can bubble. I verified in the chrome debugger that there's no stale click handler on the .unclick-me element, just on the <span>.

Stopping event propagation in the unclick handler will also prevent the click handler from being re-triggered.

[tivac]

Here's a smaller repro: https://jsbin.com/nesehey/8/edit?js,output

It attaches a non-mithril attribute to the outer node which persists between both states. Not sure if that's useful, but it is interesting.

[pygy]

Another argument for #1592

Conspiracy theory: I'm starting to think @lhorie is actually a secret Zalgo priest, luring unsuspecting coders to feed his master. The Zalgo article was mentioned in #1, and Leo is usually convinced by rational arguments, yet, maybe-sync redraws are still there, justified as an optimization, and calls for always async are ignored...

Sync redraws win at best 16 ms, 8 on average on platforms that support rAF... UI changes that happen within 100ms are percieved as instantaneous... Incorrect code is infinitely slower, anyway.

[ilsenem]

https://codepen.io/ilsenem/pen/JEeYzx?editors=0010

This behavior is not the case in v0.2.3 with the same code base.

It may be fast redraw as @tivac mentioned.

[dead-claudia]

@pygy

Conspiracy theory: I'm starting to think @lhorie is actually a secret Zalgo priest [...]

He must've been teaming up with this guy at one point! 😆

---

In reality, I think it took the monster of 0.2 for him to start becoming a little more conservative when it comes to architecting the code. That old code base was almost the epitome of Zalgo. There was enough spaghetti logic to make improvement nearly impossible without breaking everything, over and over and over. (And this is why it wound up with a full revert - it was a read-only code base, where only the slightest change in seemingly unrelated areas caused an entire mountain to topple.)

[dead-claudia]

As for the current rewrite, I just haven't had the chance to take a decent look at it for performance tweaks.

[barneycarroll]

@isiahmeadows this particular variant of Zalgo results from a consumer API contract that occasionally deviates from otherwise predictable resolution expectations - specifically: mostly asynchronous but sometimes synchronous. Like the unreliable attempt by m.request to delay redraw until all derivative thens have resolved, the 'occasionally sync' redraw is a whimsical feature with no use case, no documentation, and the potential to create incredibly confusing and difficult to debug expectation shortfalls.

[pygy]

I'd say that here, Mithril unleashes Zalgo on itself by calling said public API in the event handler...

A cheap "fix" would be to setTimeout(m.redraw) in the event handler wrapper...

[v4n]

@pygy based on your early comment, I'm assuming #1592 would fix this bug?

@isiahmeadows @tivac mind adding a bug label to this if you guys agree it's?

[pygy]

@v4n it would, but I'd rather have #1643 merged first. Both PRs modify the tests in a way that causes conflicts and #1592 will be easier to rework than #1643. Adding a node there...

[pygy]

@v4n if this is blocking you, you can add

e.redraw = false
setTimeout(m.redraw)

to the problematic handlers

Live: https://codepen.io/anon/pen/EZqrMQ?editors=0010

[dead-claudia]

Found a suspect: this branch should have an else.

[pygy]

@isiahmeadows isn't covered here?

[dead-claudia]

No, but my initial suspicion is wrong.

[dead-claudia]

The problem is that the inexistence isn't being handled correctly.

[dead-claudia]

Another suspect: this function should clean up removed attributes as well.

[dead-claudia]

It's not a 1.1 blocker, though (although it still repros).

[dead-claudia]

Closing in favor of #1804.