change code to use `.addEventListener` instead? #103

serapath · 2017-12-06T16:46:13Z

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 :-)

The text was updated successfully, but these errors were encountered:

shama · 2017-12-06T17:20:43Z

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);

bcomnes · 2017-12-06T17:27:21Z

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.

serapath · 2017-12-06T17:28:59Z

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

bcomnes · 2017-12-06T17:32:41Z

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?

serapath · 2017-12-06T19:34:18Z

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

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.
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

button.addEventListener('click', fn)

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

goto-bus-stop · 2017-12-06T20:11:29Z

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

serapath · 2017-12-06T20:37:57Z

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

button.addEventListener('click', fn)

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

serapath · 2017-12-07T16:22:30Z

@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:

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
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 mentioned this issue Dec 6, 2017

use addEventListener to allow { handleEvent } #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change code to use `.addEventListener` instead? #103

change code to use `.addEventListener` instead? #103

serapath commented Dec 6, 2017

shama commented Dec 6, 2017 •

edited

bcomnes commented Dec 6, 2017 •

edited

serapath commented Dec 6, 2017

bcomnes commented Dec 6, 2017

serapath commented Dec 6, 2017 •

edited

goto-bus-stop commented Dec 6, 2017

serapath commented Dec 6, 2017

serapath commented Dec 7, 2017 •

edited

change code to use .addEventListener instead? #103

change code to use .addEventListener instead? #103

Comments

serapath commented Dec 6, 2017

shama commented Dec 6, 2017 • edited

bcomnes commented Dec 6, 2017 • edited

serapath commented Dec 6, 2017

bcomnes commented Dec 6, 2017

serapath commented Dec 6, 2017 • edited

button.onclick = fn

button.addEventListener('click', fn)

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

goto-bus-stop commented Dec 6, 2017

serapath commented Dec 6, 2017

button.onclick = fn

button.addEventListener('click', fn)

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

serapath commented Dec 7, 2017 • edited

change code to use `.addEventListener` instead? #103

change code to use `.addEventListener` instead? #103

shama commented Dec 6, 2017 •

edited

bcomnes commented Dec 6, 2017 •

edited

serapath commented Dec 6, 2017 •

edited

serapath commented Dec 7, 2017 •

edited