karaxnim / karax

Karax. Single page applications for Nim.
MIT License
1.05k stars 90 forks source link

inline closures in loop all capture the last instance for onclick #282

Closed chancyk closed 3 months ago

chancyk commented 3 months ago

EDIT: This doesn't actually seem to be an issue, just a misunderstanding of the behavior in Nim on my part I think.

Nim : v2.0.2 Karax : v1.3.3 OS: Windows 11

import karax/[vdom, karax, karaxdsl]

type

    Thing = ref object
        val: int
    State = ref object
        things: seq[Thing]

var state = State(things: @[
    Thing(val: 0),
    Thing(val: 1)
])

proc update(t: Thing) =
    t.val += 1

proc makeBrokenButtons(): VNode =
    result = buildHtml(tdiv):
        for t in state.things:
            # ISSUE: Creating the closure inline causes the last instance of t
            # to be captured for every button.
            tdiv(class="thing", onclick=proc() = update(t)):
                text t.val.repr

proc makeWorkingButtons(): VNode =
    # WORKING: Creating a proc that creates the closure instead works as expected.
    proc updateThing(t: Thing): proc() =
        result = proc() = update(t)

    result = buildHtml(tdiv):
        for t in state.things:
            tdiv(class="thing", onclick=updateThing(t)):
                text t.val.repr

when isMainModule:
    setRenderer makeBrokenButtons

I'm not sure if this would be expected behavior for some reason due to the nature of the DOM creation. See the comments above marking the issue and the working version.

chancyk commented 3 months ago

Here's the relevant compiled javascript.

Working Example

function makeWorkingButtons_520093850() {

  function updateThing_520093852(t_520093853) {

    function HEX3Aanonymous_520093856() {
      var F = {procname: "updateThing.:anonymous", prev: framePtr, filename: "c:\\Projects\\fitness\\issues\\binding.nim", line: 0};
      framePtr = F;
        F.line = 30;
        F.filename = "binding.nim";
        update_520093712(t_520093853);
      framePtr = F.prev;

    }

    var result_520093855 = null;

    var F = {procname: "makeWorkingButtons.updateThing", prev: framePtr, filename: "c:\\Projects\\fitness\\issues\\binding.nim", line: 0};
    framePtr = F;
      F.line = 30;
      F.filename = "binding.nim";
      result_520093855 = HEX3Aanonymous_520093856;
    framePtr = F.prev;

    return result_520093855;

  }

  var result_520093851 = null;

  var F = {procname: "binding.makeWorkingButtons", prev: framePtr, filename: "c:\\Projects\\fitness\\issues\\binding.nim", line: 0};
  framePtr = F;
    F.line = 32;
    F.filename = "binding.nim";
    F.line = 32;
    var tmp_520093857 = tree_671089495(44, []);
    Label1: {
      F.line = 33;
      var t_520093862 = null;
      F.line = 241;
      F.filename = "iterators.nim";
      var i_520094088 = 0;
      F.line = 242;
      var L_520094089 = (state_520093711[0].things).length;
      Label2: {
        F.line = 243;
          Label3: while (true) {
          if (!(i_520094088 < L_520094089)) break Label3;
            F.line = 33;
            F.filename = "binding.nim";
            t_520093862 = state_520093711[0].things[chckIndx(i_520094088, 0, (state_520093711[0].things).length - 1)];
            F.line = 34;
            var tmp_520093858 = tree_671089495(44, []);
            F.line = 34;
            tmp_520093858.class = "thing";
            F.line = 33;
            addEventHandler_1056966378(tmp_520093858, 0, updateThing_520093852(t_520093862), kxi__);
            F.line = 33;
            add_671089431(tmp_520093858, text_671089563(reprInt(t_520093862.val)));
            F.line = 33;
            add_671089431(tmp_520093857, tmp_520093858);
            F.line = 245;
            F.filename = "iterators.nim";
            i_520094088 = addInt(i_520094088, 1);
            if (!(((state_520093711[0].things).length == L_520094089))) {
            F.line = 246;
            failedAssertImpl_268435541(makeNimstrLit("C:\\Users\\chanc\\.choosenim\\toolchains\\nim-2.0.2\\lib\\system\\iterators.nim(246, 11) `len(a) == L` the length of the seq changed while iterating over it"));
            }

          }
      };
    };
    result_520093851 = tmp_520093857;
  framePtr = F.prev;

  return result_520093851;

}

Broken Example

function makeBrokenButtons_520093718() {

  function HEX3Aanonymous_520093783() {
    var F = {procname: "makeBrokenButtons.:anonymous", prev: framePtr, filename: "c:\\Projects\\fitness\\issues\\binding.nim", line: 0};
    framePtr = F;
      F.line = 25;
      F.filename = "binding.nim";
      update_520093712(t_520093779);
    framePtr = F.prev;
  }

  var result_520093719 = null;

  var F = {procname: "binding.makeBrokenButtons", prev: framePtr, filename: "c:\\Projects\\fitness\\issues\\binding.nim", line: 0};
  framePtr = F;
    F.line = 22;
    F.filename = "binding.nim";
    F.line = 22;
    var tmp_520093750 = tree_671089495(44, []);
    Label1: {
      F.line = 23;
      var t_520093779 = null;
      F.line = 241;
      F.filename = "iterators.nim";
      var i_520094088 = 0;
      F.line = 242;
      var L_520094089 = (state_520093711[0].things).length;
      Label2: {
        F.line = 243;
          Label3: while (true) {
          if (!(i_520094088 < L_520094089)) break Label3;
            F.line = 23;
            F.filename = "binding.nim";
            t_520093779 = state_520093711[0].things[chckIndx(i_520094088, 0, (state_520093711[0].things).length - 1)];
            F.line = 23;
            rawEcho(reprAny(t_520093779, null, NTI520093742));
            F.line = 25;
            var tmp_520093755 = tree_671089495(44, []);
            F.line = 25;
            tmp_520093755.class = "thing";
            F.line = 25;
            addEventHandler_1056966378(tmp_520093755, 0, HEX3Aanonymous_520093783, kxi__);
            F.line = 26;
            add_671089431(tmp_520093755, text_671089563(reprInt(t_520093779.val)));
            F.line = 26;
            add_671089431(tmp_520093750, tmp_520093755);
            F.line = 245;
            F.filename = "iterators.nim";
            i_520094088 = addInt(i_520094088, 1);
            if (!(((state_520093711[0].things).length == L_520094089))) {
            F.line = 246;
            failedAssertImpl_268435541(makeNimstrLit("C:\\Users\\chanc\\.choosenim\\toolchains\\nim-2.0.2\\lib\\system\\iterators.nim(246, 11) `len(a) == L` the length of the seq changed while iterating over it"));
            }

          }
      };
    };
    result_520093719 = tmp_520093750;
  framePtr = F.prev;

  return result_520093719;

}
chancyk commented 3 months ago

And here's a more concise javascript example that reproduces the capturing behavior based on the above compiled js from Nim:

var thing1 = {
    val: 0
}
var thing2 = {
    val: 0
}
var things = [thing1, thing2];
var state = {
    things: things
}

function update(thing) {
    thing.val += 1;
}

var thing = null;

function anonymous() {
    update(thing);
}

var events = [];
function capture(fn) {
    events.push(fn);
}

function updateThing(thing) {
    return () => update(thing);
}

var i = 0;
while (true) {
    if (!(i < things.length)) break;
    console.log(i);
    thing = things[i];
    capture(anonymous);        // BROKEN BEHAVIOR
    //capture(updateThing(thing));  // WORKING BEHAVIOR
    i += 1;
}

for (let fn of events) {
    fn();
}

console.log("Thing1: ", thing1.val);
console.log("Thing2: ", thing2.val);

So it seems like it's this behavior of going through the stack as a parameter versus binding to a variable in the same scope.

chancyk commented 3 months ago

This behavior seems consistent with Nim though, so I guess it's not actually an issue:


type

    Thing = ref object
        val: int
    State = ref object
        things: seq[Thing]

var state = State(things: @[
    Thing(val: 0),
    Thing(val: 0)
])

proc update(thing: Thing) =
    thing.val += 1

proc updateThing(thing: Thing): proc() =
    result = proc() = update(thing)

var events: seq[proc()] = @[]
for t in state.things:
    events.add  proc() = update(t)   # both update state.things[1]
    # events.add updateThing(t)      # updates state.things[0] and state.things[1]

for event in events:
    event()

echo "Thing1: ", state.things[0].val
echo "Thing2: ", state.things[1].val
Araq commented 3 months ago

Yup, it's a Nim issue.