Skip to content
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

Intervening elements are re-rendered if an element is added both before and after them #56

Closed
akbcode opened this issue Mar 22, 2018 · 1 comment · Fixed by #122
Closed

Comments

@akbcode
Copy link

akbcode commented Mar 22, 2018

This example might explain better

include karax / prelude
import future
var page = 1

proc createDom(): VNode = buildHtml(tdiv):
  h1:
    text "Testing"
  if page == 3 or page == 4:
    tdiv: text "Some text"
  input(`type`="text")
  button(onclick=() => (page = 1)): text "1"
  button(onclick=() => (page = 2)): text "2"
  button(onclick=() => (page = 3)): text "3"
  button(onclick=() => (page = 4)): text "4"
  if page == 2 or page == 4:
    tdiv: text "Some text"
setRenderer createDom

Going from page 1 to page 2 or page 1 to page 3 works as expected. But page 1 to page 4 causes the input and buttons 1, 2 and 3 to be re-rendered.

@timotheecour
Copy link
Collaborator

timotheecour commented Sep 13, 2019

just ran into this and was puzzled by the results; this is a serious bug...
I verified it's still there on master

here's a reduced case:

  include karax / prelude
  import sugar
  var page = 2
  proc createDom(): VNode = buildHtml(tdiv):
    if page == 3:
      tdiv: text "Some text v3"
    # else: tdiv: text "Some text v2" # uncomment would remove bug
    button(onclick=() => (echo "click 2"; page = 2)): text "2"
    button(onclick=() => (echo "click 3"; page = 3)): text "3"
    tdiv: text "Some text2: " & $page # removing `& $page` would remove bug
  setRenderer createDom

click 3, then click 2: it should print:

click 3
click 2

but prints:

click 3
click 3

looks like dom diffing gets confused by the conditional inclusion of the node tdiv: text "Some text v3"

EDIT

just checked, this is actually a recent regression caused by e422087 /cc @Araq

  • works in d08a035, both this example and the one in top post, so this bug had actually been fixed until it got broken again
  • fails in e422087 avoid exponential DOM diffing
    (although e422087 did help for another bug, as i had mentioned here e422087#commitcomment-35065660)

EDIT:

  • adding let recursive=true right below proc eq(a, b: VNode; recursive: bool): EqResult = makes the bug go away; but that probably is bad for performance (maybe exponential complexity)
  • attaching a (unique) id on each button makes the bug go away; obviously this isn't desirable but could help debugging that issue; maybe what's going on is:
    inside karax.eq, when recursive=false, button 2 and button 3 might get confused; the even listeners are not compared (see # Do not test event listeners here!); not sure how to compare them though, the naive way would likely just always return "different" even if comparing the same nodes
  include karax / prelude
  import sugar
  var page = 2
  proc createDom(): VNode = buildHtml(tdiv):
    if page == 3:
      tdiv: text "Some text v3"
    # else: tdiv: text "Some text v2" # uncomment would remove bug
    button(id = "id1", onclick=() => (echo "click 2"; page = 2)): text "2"
    button(id = "id2", onclick=() => (echo "click 3"; page = 3)): text "3"
    tdiv: text "Some text2: " & $page # removing `& $page` would remove bug
  setRenderer createDom

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

Successfully merging a pull request may close this issue.

2 participants