Simplified event handlers - Option 1. #235

l-cornelius-dol · 2021-11-17T05:22:42Z

This change simplifies event handlers to be immediately attached using onxxx attributes, with a fixed DOMVM intermediary which invokes the VM and global onevent handlers.

The memory and performance comparisons follow (with 1,000,000 elements).

Vanilla JS

No handlers:

    <script>
      console.time("nohandler");
      for (let i = 0; i < 1000000; i++) {
        let elm = document.createElement("span");
        elm.textContent = "["+i+"]";
        document.body.appendChild(elm);
      }
      console.timeEnd("nohandler");
    </script>

Measured Time: 625 ms

With `onclick` handlers:

    <script>
    function onclick() {}

    console.time("onx");
    for (let i = 0; i < 1000000; i++) {
        let elm = document.createElement("span");
        elm.textContent = "["+i+"]";
        elm.onclick = onclick;
        document.body.appendChild(elm);
        }
    console.timeEnd("onx");
    setTimeout(() => { document.body.innerHTML = "<h1>Done!</h1>"; },1000);
    </script>

Measured Time: 1816

DOMVM

No event handlers:

<script>
const ve = domvm.defineElement;

let done = false;

function AppView() {
    return () => {
        if(done) { return ve("h1","DONE!"); }

        let chl = [];
        for(let xa = 0; xa < 1000000; xa++) {
            chl.push(ve("span", "["+xa+"]"));
            }
        return ve("div",chl);
        }
    }

console.time("domvm");
let rootVm = domvm.createView(AppView);
rootVm.cfg({
    hooks: {
        didMount: (vm) => {
            console.timeEnd("domvm");
            setTimeout(() => { done = true; vm.redraw() },1000);
            },
        }
    });
rootVm.mount(document.body);
</script>

Measured Time: 6305

With `onclick` handlers:

<script>
const ve = domvm.defineElement;

let done = false;

function onclick() {}

function AppView() {
    return () => {
        if(done) { return ve("h1","DONE!"); }

        let chl = [];
        for(let xa = 0; xa < 1000000; xa++) {
            chl.push(ve("span", { onclick: onclick }, "["+xa+"]"));
            }
        return ve("div",chl);
        }
    }

console.time("domvm");
let rootVm = domvm.createView(AppView);
rootVm.cfg({
    hooks: {
        didMount: (vm) => {
            console.timeEnd("domvm");
            setTimeout(() => { done = true; vm.redraw() },1000);
            },
        }
    });
rootVm.mount(document.body);
</script>

Measured Time: 7344

l-cornelius-dol · 2021-11-17T05:26:22Z

@leeoniya

Testing with my jsfiddle locally shows exactly the desired behavior and sequence, now identical whether a "normal" or "parameterized handler.

<script>
const ve = domvm.defineElement,
      vw = domvm.defineView;

let message  = [];
let redraw = function() {
    let pnd = 0;
    return () => {
    	if(!pnd) { pnd = setTimeout(() => { pnd = 0; rootVm.redraw(true); }); }
        }
    }();


function AppView(vm) {
	return {
    	init: function() {
        	vm.cfg({ onevent: vmHandler });
            },

        render: function appView() {
            try {
                return ve("div", [
                    ve("div", { onclick: evtOuter }, [
                        ve("div", { onclick: [ evtOuter,true ] }, [
                            ve("button", { onclick: evtButton }, "Outer Button"),
                            vw(InnerView)
                            ]),
                        ]),
                    ve("ol", message.map((msg) => ve("li",msg))),
                    ]);
                }
            finally {
                message = [];
                }
    		},
        };
    }

function InnerView(vm) {
	return {
    	init: function() {
        	vm.cfg({ onevent: vmHandler });
            },

        render: function innerView() {
            return ve("div", [
                ve("span", { onclick: evtInner }, [
                    ve("span", { onclick: [ evtInner,true ] }, [
                        ve("button", { onclick: evtButton }, "Inner Button"),
                        ]),
                    ]),
                ]);
            },
        };
	}

function evtButton(evt) {
    message.push("Button Handler: "+evt.target.innerHTML);
    }

function evtOuter(flg) {
    message.push("Outer Handler: "+(flg===true ? "With Args" : "Function"));
    redraw();
	}

function evtInner(flg) {
    message.push("Inner Handler: "+(flg===true ? "With Args" : "Function"));
    redraw();
	}

function vmHandler(evt,nod,vm) {
    message.push("VM Handler: "+vm.view.name);
    redraw();
    }

function globalHandler(evt,nod,vm) {
	message.push("Global Handler: "+vm.view.name);
    redraw();
    }

domvm.config({ onevent: globalHandler });
let rootVm = domvm.createView(AppView).mount(document.body);
</script>

l-cornelius-dol · 2021-11-17T07:05:36Z

And the obvious comparison, against 3.4.12 parameterized handlers:

<script>
const ve = domvm.defineElement;

let done = false;

function onclick() {}

function AppView() {
    return () => {
        if(done) { return ve("h1","DONE!"); }

        let chl = [];
        for(let xa = 0; xa < 1000000; xa++) {
            chl.push(ve("span", { onclick: [onclick] }, "["+xa+"]"));
            }
        return ve("div",chl);
        }
    }

console.time("domvm");
let rootVm = domvm.createView(AppView);
rootVm.cfg({
    hooks: {
        didMount: (vm) => {
            console.timeEnd("domvm");
            setTimeout(() => { done = true; vm.redraw() },1000);
            },
        }
    });
rootVm.mount(document.body);
</script>

Measured Time: 6044

l-cornelius-dol · 2021-11-17T07:28:56Z

Finally, with DOMVM and my other pull-request, which adds a listen to each VM's element. It's worth noting here that memory consumption does not appear to change whether 1 or 1,000,000 listeners are used, which is consistent with my expectations that the onclick pointer is the same whether null or set to a value.

<script>
const ve = domvm.defineElement;

let done = false;

function onclick() {}

function AppView() {
    return () => {
        if(done) { return ve("h1","DONE!"); }

        let chl = [];
        for(let xa = 0; xa < 1000000; xa++) {
            chl.push(ve("span", { onclick: [onclick] }, "["+xa+"]"));
            }
        return ve("div",chl);
        }
    }

console.time("domvm");
let rootVm = domvm.createView(AppView);
rootVm.cfg({
    hooks: {
        didMount: (vm) => {
            console.timeEnd("domvm");
            setTimeout(() => { done = true; vm.redraw() },1000);
            },
        }
    });
rootVm.mount(document.body);
</script>

Measured Time: 6793

l-cornelius-dol · 2021-11-17T07:36:56Z

Personally, I think the relatively tiny added time (it is after all about 1us per event handler on my HEDT) is probably well worth the total consistency with the native event system. But it should definitely be measured on other systems as well. And en toto performance should be gauged (there may well be other hidden improvements offsetting the added time here).

The handler attach time will be dwarved into insignificance by the construction of a real-life DOM where the handler will represent a tiny part of the overall work -- this synthetic test magnifies the cost of adding an event handler (by intentional design).

leeoniya · 2021-11-18T02:27:02Z

hey @lawrence-dol

i've pushed some changes on top of your work in https://github.com/domvm/domvm/tree/onx-events

style cleanups
there is no longer handler re-assigning on no-op redraw, this eliminated the handler growth in chrome console (don't ask me why it was growing when re-attaching same fn, but it was!)
i've adjusted some tests to expect onclick reassignments instead of add/removeEventListener

please maintain the existing style of the rest of the codebase (commas, alignment, spacing, prefer let instead of var, etc). it should look like it was written by one person.

overall this looks pretty promising. my biggest concern, as i've stated before, is that non-parametrized handlers should be invoked as they would in raw dom. we can still bind our proxy handler, but it should invoke the original handlers with the raw event and properly-bound this. let's leave all the special magic in the domain of parametrized handlers. i think once this is done, it will fix the remaining failing tests that now fail due to wrong parameter counts, and additional invocation of global and vm onevent handlers.

let me know how this works for you.

here's what i'm using for local testing:

<!doctype html>
<html>
	<head>
		<title>Perf test</title>
		<script src="./dist/nano/domvm.nano.iife.js"></script>
	</head>
	<body>
		<script>
			let {
				createView: cv,
				defineView: vw,
				defineElement: el,
			} = domvm;

			let App = (vm, data) => {
				let showAll = true;

				const toggle = e => {
				//	showAll = !showAll;
					vm.redraw();
				};

				return (vm, data) => (
					el("div", [
						el("button", {onclick: toggle}, "Toggle"),
						showAll ? vw(Table, data) : null,
					])
				);
			};

			let Table = (vm, data) => {
				let onclick = (v, i) => {
					console.log(i, v);
				};

				return (vm, data) => (
					el("table", data.map((v, i) =>
						el("tr", [
							el("td", {onclick: [onclick, v, i]}, v)
						])
					))
				);
			};

			const data = Array.from({length: 3500}, () => Math.floor(Math.random() * 1000));

			cv(App, data).mount(document.body);
		</script>
	</body>
</html>

leeoniya · 2021-11-18T02:58:27Z

P.S. we may need to switch back to using add/removeEventListener, i remember back in the day that there were some events that had no equivalent assignable on* properties, though i forget which ones they were (i thought maybe transitionend, but that looks ok [1]).

we will also have to go through all the demos in the playground and make sure they still work as before, especially ones that rely on event bindings: https://domvm.github.io/domvm/demos/

[1] https://developer.mozilla.org/en-US/docs/Web/Events

l-cornelius-dol · 2021-11-18T03:05:12Z

non-parametrized handlers should be invoked as they would in raw dom. we can still bind our proxy handler, but it should invoke the original handlers with the raw event and properly-bound this.

Completely agree, and tried to do that, at least with respect to this and the event argument.

When I was looking into tests, I found I had unintentionally broken compatibility with the onevent invocations; you may have already noticed and fixed this, but just in case you hadn't. Specifically, the args are passed as an array, and my change made them added, separate arguments. Exec should be changed back to (I think):

function exec(fn, args, e, evnode) {
    let vm = getVm(node),
        out1 = fn.apply(e.currentTarget, args.concat(e, evnode, vm, vm.data)) // this == currentTarget, NOT vm, to match normal handler
        out2, 
        out3;

    if (FEAT_ONEVENT) {
        out2 = vm.onevent(e, evnode, vm, vm.data, args),
	out3 = onevent.call(null,e, evnode, vm, vm.data, args);
    }

    if (out1 === false || out2 === false || out3 === false) {
	e.preventDefault();
	e.stopPropagation(); // compatibility; this is almost never the right thing to do
	return false;
    }
}

l-cornelius-dol · 2021-11-18T03:15:45Z

we may need to switch back to using add/removeEventListener

That rings a bell with me too, now that you mention it. As simple as:

    if (nval == null)
        el.removeEventListener(name,handle);
    else if (oval == null)
        el.addEventListener(name,handle);

no?

leeoniya · 2021-11-18T03:42:17Z

you'd need to do name.slice(2) if switching to add/remove. i also found add/remove to be somewhat slower than prop assignment.

let me know what you find, but unless we can pinpoint which events have no on equivalents these days, maybe staying with event properties is better.

l-cornelius-dol · 2021-11-18T04:22:46Z

you'd need to do name.slice(2) if switching to add/remove. i also found add/remove to be somewhat slower than prop assignment.

So I just found out. I pushed what I think are the final three changes on my branch. (Sorry if I've done anything incorrectly, here -- this is my first time collaborating like this on github).

let me know what you find, but unless we can pinpoint which events have no on equivalents these days, maybe staying with event properties is better.

I can't detect any material difference between onxxx and add/remove listener. I think it should be onxxx because (a) that's what the template declared, (b) I prefer the simplicity of onxxx, and (c) MDN implies that the only (relevant) drawback of onxxx handlers is that they only work for HTML and SVG elements. But at that point, the programmer is breaking out of onxxx and using add/remove listener themselves, though the case could be made that we can better their lives, I suppose..

In the end, I am ambivalent and defer to your judgement on the matter. Feel free to accept or reverse that commit.

l-cornelius-dol · 2021-11-18T04:24:39Z

we will also have to go through all the demos in the playground and make sure they still work as before

I'm happy to help with this to the extent I can (I don't necessary know what to expect, and my not notice if anything works incorrectly). Just point me in the right direction.

But for today, I am wrecked, so I'm packing it in until tomorrow.

leeoniya · 2021-11-18T06:27:21Z

i've added the extra changes for raw events and merged this work into master. the tests and demos pass from my testing.

i've also updated the dependencies and build script so you'll need to re-run npm i to sync after pulling master.

any followup work should be done in a new PR. but feel free to continue discussing/planning in this thread.

the only real behavioral change affected whose vnode is supplied as the node argument to parameterized handlers. previously it was the vnode attached to e.target (or its closest vnode ancestor, in case the target was an injected raw dom element...similar to getVm()). with these changes it becomes e.currentTarget._node. this is not technically a required change and we can still re-create the prior behavior easily by crawling upwards from e.target. i still think it's more useful to have direct access to the target's vnode than to the vnode of the element with the handler, but wanted to see what you thought and whether we should roll it back or if it's more expected/uniform the new way. the relevant test changes were done in 13d3ac5 -- you can see that previously the click handler attached to the parent expected to receive the vnode of the clicked e.target._node descendant.

l-cornelius-dol · 2021-11-18T08:03:35Z

Looks great. Thanks!

i still think it's more useful to have direct access to the target's vnode than to the vnode of the element with the handler

I'll sleep on it, and look over some event handling code tomorrow to get a more concrete understanding instead of merely theorizing -- you might well be right. Will try to have a firm opinion by EOB tomorrow.

l-cornelius-dol · 2021-11-18T20:27:55Z

i still think it's more useful to have direct access to the target's vnode than to the vnode of the element with the handler

After reviewing a wide range of existing code, it seems I am about equally often interested in the target as I am the currentTarget, implying equal interest in the target vnode and current-target vnode. Actually, I tend to get the vnode through the element, when needed, without using the special added arguments at all -- I don't like the incongruence between "normal" and "parameterized" handlers, so I code as if the only arguments are the event and those I explicitly injected (I'll have more to say on this via a separate issue).

Therefore, my conclusion is that it's a wash in terms of usefulness of one over the other, and backward compatibility dictates it should remain as it was, the target vnode.

Could you please proceed with a new release as soon as you are satisfied it's ready?

leeoniya · 2021-11-18T20:56:37Z

ok, thanks. i'll let it simmer over the weekend and see how i feel about it in a couple days. but like you said, backwards compat will probably be the tie-breaker.

in the mean time, please test the hell out of the new implementation and let me know how it functions and performs relative to your expectations; you're likely the only domvm user with such intricate event handling requirements.

l-cornelius-dol · 2021-11-21T00:39:09Z

I've integrated the new build into several apps, with a mix of handlers, and so far have observed no adverse effects. I do have to be a little bit cautious, as all my apps are in production and I don't want to prematurely deploy code. But so far, so good.

l-cornelius-dol · 2021-12-03T00:05:52Z

@leeoniya : Could you release 3.4.13 now? Also, 3.4.12 was never released here on Git Hub.

gdixon · 2021-12-03T03:36:08Z

I love seeing you guys in my inbox! Good work the pair of you 🚀

leeoniya · 2021-12-08T03:09:07Z

sorry a bit busy this week with work. will take a look over weekend.

l-cornelius-dol · 2022-01-13T04:03:35Z

@leeoniya : Reminding you about 3.4.13.

l-cornelius-dol · 2022-01-30T00:24:09Z

@leeoniya : Reminding you again about releasing the next version. I need to put this production in the next few days and would like to do so with an "official" build.

I'm open to doing it myself, if you have instructions and want to enable me to do so.

leeoniya · 2022-01-31T06:17:47Z

sorry! will tag/push a release tomorrow.

leeoniya · 2022-02-01T02:37:26Z

3.4.13 is published

Simplified event handlers.

1c5403f

l-cornelius-dol changed the title ~~Simplified event handlers.~~ Simplified event handlers - Option 1. Nov 17, 2021

leeoniya added 2 commits November 17, 2021 19:43

tweaks and pokes

d1e8f75

some test fixups

52d1238

leeoniya mentioned this pull request Nov 18, 2021

Simplified event handlers - Option 2. #236

Closed

l-cornelius-dol added 3 commits November 17, 2021 20:13

Set event handler this to e.currentTarget.

365bafd

Restore onevent args to be an array instead of separate arguments.

1e8c9c7

Revert to using add/remove listener.

21ccecc

leeoniya closed this Nov 18, 2021

l-cornelius-dol mentioned this pull request Nov 18, 2021

Parameterized Event Handler Invocation #231

Closed

l-cornelius-dol deleted the onx-events branch November 18, 2021 21:29

l-cornelius-dol mentioned this pull request Nov 21, 2021

Incongruence of Normal and Parameterized Handlers #237

Open

l-cornelius-dol mentioned this pull request Feb 15, 2022

Backward Compatibility Bug from #235 #240

Closed

l-cornelius-dol mentioned this pull request Mar 15, 2022

Backward Compatibility Bug (2) from #235 #244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified event handlers - Option 1. #235

Simplified event handlers - Option 1. #235

l-cornelius-dol commented Nov 17, 2021

l-cornelius-dol commented Nov 17, 2021 •

edited

l-cornelius-dol commented Nov 17, 2021

l-cornelius-dol commented Nov 17, 2021

l-cornelius-dol commented Nov 17, 2021

leeoniya commented Nov 18, 2021 •

edited

leeoniya commented Nov 18, 2021 •

edited

l-cornelius-dol commented Nov 18, 2021

l-cornelius-dol commented Nov 18, 2021

leeoniya commented Nov 18, 2021 •

edited

l-cornelius-dol commented Nov 18, 2021 •

edited

l-cornelius-dol commented Nov 18, 2021

leeoniya commented Nov 18, 2021

l-cornelius-dol commented Nov 18, 2021

l-cornelius-dol commented Nov 18, 2021 •

edited

leeoniya commented Nov 18, 2021

l-cornelius-dol commented Nov 21, 2021

l-cornelius-dol commented Dec 3, 2021

gdixon commented Dec 3, 2021

leeoniya commented Dec 8, 2021

l-cornelius-dol commented Jan 13, 2022

l-cornelius-dol commented Jan 30, 2022

leeoniya commented Jan 31, 2022

leeoniya commented Feb 1, 2022

Simplified event handlers - Option 1. #235

Simplified event handlers - Option 1. #235

Conversation

l-cornelius-dol commented Nov 17, 2021

Vanilla JS

No handlers:

With onclick handlers:

DOMVM

No event handlers:

With onclick handlers:

l-cornelius-dol commented Nov 17, 2021 • edited

l-cornelius-dol commented Nov 17, 2021

l-cornelius-dol commented Nov 17, 2021

l-cornelius-dol commented Nov 17, 2021

leeoniya commented Nov 18, 2021 • edited

leeoniya commented Nov 18, 2021 • edited

l-cornelius-dol commented Nov 18, 2021

l-cornelius-dol commented Nov 18, 2021

leeoniya commented Nov 18, 2021 • edited

l-cornelius-dol commented Nov 18, 2021 • edited

l-cornelius-dol commented Nov 18, 2021

leeoniya commented Nov 18, 2021

l-cornelius-dol commented Nov 18, 2021

l-cornelius-dol commented Nov 18, 2021 • edited

leeoniya commented Nov 18, 2021

l-cornelius-dol commented Nov 21, 2021

l-cornelius-dol commented Dec 3, 2021

gdixon commented Dec 3, 2021

leeoniya commented Dec 8, 2021

l-cornelius-dol commented Jan 13, 2022

l-cornelius-dol commented Jan 30, 2022

leeoniya commented Jan 31, 2022

leeoniya commented Feb 1, 2022

With `onclick` handlers:

With `onclick` handlers:

l-cornelius-dol commented Nov 17, 2021 •

edited

leeoniya commented Nov 18, 2021 •

edited

leeoniya commented Nov 18, 2021 •

edited

leeoniya commented Nov 18, 2021 •

edited

l-cornelius-dol commented Nov 18, 2021 •

edited

l-cornelius-dol commented Nov 18, 2021 •

edited