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

UIHandler: Event delegation, introducing DOM handleEvent *) #400

Closed
wants to merge 1 commit into from

Conversation

filiphlm
Copy link

I have no ambition to "fix" anything, I am giving purely for consideration/discussion a modification that has worked well for me in an AJX-heavy / custom elements riddled application.

  • Memory consumption--
  • Simplified code (one handleEvent to rule them all), no manual binding, no unreachable bound methods
  • Performance++ (This may not be obvious with the default .ajax selector, but with something like naja.uiHandler.selector = ':not([data-naja="off"])' — completely different situation.)
  • Also: events are easily removed with removeEventListener('click', naja.uiHandler). If you need to.

BC Break: The bindUI function has been removed as there is no further use for it.

*) Since 2000; see: https://dom.spec.whatwg.org/#interface-eventtarget

@jiripudil
Copy link
Member

jiripudil commented May 16, 2024

Hi,

first of all, sorry for the delayed response, I was taking a week off and then a week of catching up 😬

Thanks for opening the topic!

I prefer Naja's UI binding the way it is because it feels more transparent and straight-forward. I always believed performance implications to be negligible for the majority of use cases, and honestly, I still do.

That said, I understand the impact on performance may grow when your application gets complex enough to contain many interaction targets. I was in fact considering event delegation before, but decided to leave this for when somebody actually comes up with the issue. And here we are :)

Re your PR: I think it tries to do two unrelated things in one sweep:

  • handleEvent - to be honest, I don't see the value in this change. It is my impression that developers (me included) are much more used to function listeners than objects implementing handleEvent(). Thus, to me, the code is more readable when there is a direct, explicit reference to the handler function rather than this. The listener itself is currently stored in this.handler, so the memory footprint is constant anyway.

  • As to event delegation, I'm definitely not opposed to the idea, but I'd prefer to keep the current behaviour as the default, and add delegation as an opt-in for developers who run into performance issues. Implementation-wise, my rough idea is that the binding could be extracted into a strategy interface that developers could configure before initializing Naja. Would you be interested in implementing that?

@filiphlm
Copy link
Author

filiphlm commented May 16, 2024

And here we are :)

And I sincerely apologize for that.

About that article: The bottom line is, "You have to know exactly what you're doing and why you're doing it." That said, event delegation is still better/safer option in most cases, IMO. Yes, he's right about modern browsers. But still, every time you add a listener and don't reference it properly (which is unfortunately the "industry standard" approach), it's like creating a setInterval with no way to stop it. The only thing that changed over time is that we went from setInterval(()=>{}, 200) to setInterval(()={},10) 😄

handleEvent - to be honest, I don't see the value in this change.

I get it. I admit that there is a certain degree of convenience behind it. (Why use extra memory for a context when you have a native behavior that does it for you? And If someone accidentally adds same instance as listener twice, I'm still safe. Etc.) When I found out about this years ago, it was one of the "holy f***" moments, and I rarely use bind or arrow functions in listeners since then. But you're absolutely right, it is not necessary in the given context.

Would you be interested in implementing that?

Yes, sure. Although I have only a vague idea of the exact solution at this point.

@filiphlm filiphlm closed this May 17, 2024
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 this pull request may close these issues.

None yet

2 participants