nanohtml icon indicating copy to clipboard operation
nanohtml copied to clipboard

Published 20 hours ago • verified publisher icon choojs

change code to use `.addEventListener` instead?

Open serapath opened this issue 7 years ago • 8 comments

When using addEventListener, one could pass an object onclick=${{ handleEvent }}.

This article explains it in more details what potential benefits this can have: https://medium.com/@WebReflection/dom-handleevent-a-cross-platform-standard-since-year-2000-5bf17287fd38

I guess it would need only a single change here: https://github.com/shama/bel/blob/68b261e6892170fb972f4488ab4dfc200b125213/browser.js#L76

el[p] = val would become el.addEventListener(key.slice(2), val)

I can make the PR if this change would be desirable. If there are reasons why this would not be desirable, I'd be curious to learn about the reasons too :-)

serapath avatar Dec 06 '17 16:12 serapath

We don't handle or keep track of events. The ability to add events is just a by-product of being able to set properties through a bel tagged template string, in this case ones that prefer to be set directly on the object instead.

Also el.onclick = val will always set one listener whereas addEventListener will add multiple listeners. With a changing DOM you run the risk of adding multiple unintended listeners without providing an interface to remove them.

It would be more difficult to support addEventListener through the tagged template syntax as we begin down the road of building in an event system. I think if that is preferred by the author, they should:

const el = bel`<button>click</button>`
el.addEventListener('click', function () {}, false);

shama avatar Dec 06 '17 17:12 shama

That article is really great actually, its a glaring solution to passing context without binding, thus maintaining the prototype chain advantages (vs of 100s of bound instance functions).

The big 'secret' to it that adding the same event handler multiple times automatically de-dupes attachments somehow... so even adding multiple listeners might not be an issue, since you really are in-effect adding one. Not sure if it scales to 100s of calls or not, but it might! The complications comes from the fact you need to pass in an object with the handleEvent method.

bcomnes avatar Dec 06 '17 17:12 bcomnes

What about testing and using addEventListener only when val is not a function?

serapath avatar Dec 06 '17 17:12 serapath

More specifically, my initial idea was use addEventListener if the handler implementshandleEvent. Still curious what happens when the node is discarded. Do these eventListeners stick around? Do they need manual cleanup?

bcomnes avatar Dec 06 '17 17:12 bcomnes

I tried to explore it. I never tried that before.

To me it looks, that when you discard a node, no matter which way of adding an event handler you use, they're all removed and the memory is again released

  1. Using the performance tab in chrome <strg+e> while recording memory and then refreshing the following page with F5 shows a similar pattern after 15 seconds.
  2. Using the memory tab in chrome and take a heap snapshot

This was the code I ran and recorded for 15 seconds

I'm not sure if this is actually measuring what you are interested in. It's the first time I try to do this. If you tell me what I should change, I can measure again.

<html>
  <head>
    <title> test </title>
  </head>
  <body>
    <script>
      var n = 1000
      var str = 'foobarbaz'.repeat(n)
      var bigarray = Array.apply(null, Array(n))
      var filledarray = bigarray.map(String.prototype.valueOf,str)
      var json = JSON.stringify(filledarray)
      function makeNode () {
        var button = document.createElement('button')
        button.innerText = 'foobarbaz'


        // var fn = function listener (event) { console.log(event) }
        // fn.somethingbig = JSON.parse(json)
        // button.addEventListener('click', fn)

        // var fn = function listener (event) { console.log(event) }
        // fn.somethingbig = JSON.parse(json)
        // button.onclick = fnBound

        var obj = { handleEvent (event) { console.log(event) } }
        obj.handleEvent.somethingbig = JSON.parse(json)
        button.addEventListener('click', obj)


        return button
      }
      var counter = 10
      setTimeout(function makeManyNodes () {
        console.log(counter)
        if (!counter) return // document.body.innerHTML = ''
        counter--
        var el = makeNode()
        document.body.innerHTML = ''
        document.body.appendChild(el)
        setTimeout(makeManyNodes, 1000)
      }, 1000)
    </script>
  </body>
 </html>

Below are my measurements in a chrome incognito tab

button.onclick = fn

onclick1 onclick2

button.addEventListener('click', fn)

addevent1 addevent2

button.addEventListener('click', { handleEvent })

handleevent1 handleevent2

serapath avatar Dec 06 '17 19:12 serapath

@serapath you might be interested in #102 if you didn't see it yet!

goto-bus-stop avatar Dec 06 '17 20:12 goto-bus-stop

I saw that addEventListener is a bit faster than .onclick = ... and I tried another measurement without discarding nodes

<html>
  <head>
    <title> test </title>
  </head>
  <body>
    <script>
      var n = 1000
      var str = 'foobarbaz'.repeat(n)
      var bigarray = Array.apply(null, Array(n))
      var filledarray = bigarray.map(String.prototype.valueOf,str)
      var json = JSON.stringify(filledarray)

      var obj = { handleEvent (event) { console.log(event) } }
      obj.handleEvent.somethingbig = JSON.parse(json)

      function makeNode () {
        var button = document.createElement('button')
        button.innerText = 'foobarbaz'


        // var fn = function listener (event) { console.log(event) }
        // fn.somethingbig = JSON.parse(json)
        // button.addEventListener('click', fn)

        // var fn = function listener (event) { console.log(event) }
        // fn.somethingbig = JSON.parse(json)
        // button.onclick = fn


        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)
        button.addEventListener('click', obj)


        return button
      }
      var counter = 10
      setTimeout(function makeManyNodes () {
        console.log(counter)
        if (!counter) return // document.body.innerHTML = ''
        counter--
        var el = makeNode()
        // document.body.innerHTML = ''
        document.body.appendChild(el)
        setTimeout(makeManyNodes, 1000)
      }, 1000)
    </script>
  </body>
 </html>

button.onclick = fn

xonclick1 xonclick2

button.addEventListener('click', fn)

xaddevent1 xaddevent2

button.addEventListener('click', { handleEvent })

xhandleevent1 xhandleevent2

serapath avatar Dec 06 '17 20:12 serapath

@goto-bus-stop I just checked your PR and the issue you linked that motivated it.

I personally would prefer a more minimal change, because:

  1. i don't need to copy events, because i'm not using nanomorph or any generic mutation approach
    • component authors know which events they use
  2. i am building template snippets for components that want to re-use the same { handleEvent } object
    • it seems in your PR, each tag get's it's own { handleEvent } object

So i would just pass for any listener on${event.type} the same { handleEvent } object of the component the template belongs to. Each individual tag in that template would would then have a data-call=handlerName attribute, which makes the { handleEvent } actually pretty generic

module.exports = mycomponent
module.exports.prototype = {
  handleEvent (event) { this[event.target.dataset.call](event) }
  foobar (event) { }
  barbaz (event) { }
}
function mycomponent () {
  if (!(this instanceof mycomponent)) return new mycomponent()
  // ...
  var el = bel`
    <div class="mycomponent">
      <button data-call=foobar onclick=${this}> foobar </button>
      <button data-call=barbaz onclick=${this}> barbaz </button>
    <div>`
  // ...
}

...but of course, just passing this currently doesn't work, because bel does not support the { handleEvent } standard

serapath avatar Dec 07 '17 16:12 serapath