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

[docs] Best practices for adding event listeners to a host element #139

Closed
jolleekin opened this issue Aug 6, 2018 · 14 comments · Fixed by #413
Closed

[docs] Best practices for adding event listeners to a host element #139

jolleekin opened this issue Aug 6, 2018 · 14 comments · Fixed by #413

Comments

@jolleekin
Copy link

There are three ways to add event listeners to a host element

  1. In the constructor
  2. In ready
  3. In connectedCallback (requires removal in disconnectedCallback)

Which is the best way? Are there any memory leak issues for (1) and (2)?

I hope there would be some documentation on this topic.

@ghost
Copy link

ghost commented Aug 8, 2018

Thanks for the question, I'm working on the docs for lit-element :) What's in my draft at the moment is basically this

  • A custom element can add an event listener to itself or its own children from its constructor without any issues
  • Adding the listener in the constructor ensures that you will catch events that could possibly occur before the custom element has been added to DOM
  • If the custom element adds an event listener to anything except itself or its children (e.g. window), you should add the listener in connectedCallback and remove it in disconnectedCallback

This is based on the Polymer docs: https://www.polymer-project.org/3.0/docs/devguide/events. My understanding is that it's the same for lit-element, though I'll confirm this info before docs get published.

@AndreasGalster
Copy link

Can you explain the third point a bit? Not sure I'm getting why that is the case, would be great to understand.

As I understand it, constructor doesn't care about DOM whereas connectedCallback is after DOM attachment, so why would it matter for anything going to anything else than the actual component/children (DOM) to put it into connectedCallback?

@aadamsx
Copy link

aadamsx commented Aug 10, 2018

Can we get a preview somehow? There's very little info on on LitElement at the moment.

Docs quality can make or break a project 100%

@ghost
Copy link

ghost commented Aug 13, 2018

@AndreasGalster the following is my attempt at an explanation. Maybe @TimvdLippe might be able to correct me or clarify here.

I believe the reasoning for this recommendation is to do with removing the event listener in disconnectedCallback so that garbage collection can be done. To illustrate, here are a couple of scenarios:

Scenario 1

  1. MyElement constructor is called; ChildThing constructor is called; event listener is added to ChildThing
  2. Stuff happens, event listener fires, memory is allocated in its scope
  3. MyElement is destroyed; ChildThing is destroyed, and with it, the scope of the event listener; memory is freed up, all is well

Scenario 2

  1. MyElement constructor is called; event listener is added to Window
  2. Stuff happens, event listener fires, memory is allocated in its scope
  3. MyElement is destroyed

In Scenario 2, I am not sure when or how the memory in the scope created by the event listener on Window would get released. My understanding is that garbage collection works differently in different browsers and this scenario might not be good for memory management.

Scenario 2.A

  1. MyElement's connectedCallback fires, event listener is added to Window
  2. Stuff happens, event listener fires, memory is allocated in its scope
  3. disconnectedCallback fires, event listener is removed from Window, memory is freed up and all is well

The recommendation to use disconnectedCallback to remove event listeners added to things that aren't MyElement or its children seems clear in this scenario. As for why you would add it in connectedCallback instead of the constructor in this scenario, I can't say for sure, but it does seem nice and clean to do stuff in connectedCallback if you're going to undo it in disconnectedCallback. Maybe a scenario like the following would make it more relevant:

Scenario 3

  1. MyElement is appended to ParentThing, MyElement's connectedCallback fires, event listener is added to ParentThing
  2. Stuff happens, event listener fires, memory is allocated in its scope
  3. MyElement is moved to a new position in DOM, disconnectedCallback fires, event listener is removed from ParentThing
  4. MyElement is appended to OtherThing, connectedCallback fires again, etc

@ghost
Copy link

ghost commented Aug 13, 2018

@aadamsx I agree, and will try to publish a preview as soon as I have enough content :)

@TimvdLippe
Copy link
Contributor

@katejeffreys That sounds about right! 😄 I think the key scenario is 2.A here, where the usage of disconnectedcallback is crucial to prevent memory leaks.

@akc42
Copy link

akc42 commented Aug 16, 2018

Isn't there one more scenario where an event listener gets added - namely using on-my-custom-event on an element in the html template string literal returned from the _render function. Does this get removed automatically if the host element disconnects from the dom? Does this still work inside of loops?

It seems to me that this approach is easier than trying to add one via addEventListener

@TimvdLippe
Copy link
Contributor

@akc42 The on-* syntax is part of lit-html. This is sugar for calling addEventListener on these nodes. These listeners are therefore automatically cleaned up when the corresponding node is removed from the DOM.

@kenchris
Copy link
Contributor

@TimvdLippe not anymore, it is @ now, like @change=${...}

@sorvell sorvell added this to the 1.0.0 milestone Aug 28, 2018
@sorvell sorvell assigned ghost Aug 28, 2018
@isochronous
Copy link

isochronous commented Sep 26, 2018

I'd really love to see the readme.md updated with an actual useful example of event binding. Right now the only example is a @click handler that does nothing but call console.log. How is that helpful? Who is writing event handlers inline? Pretty much every event handler I write is written as a class method, but there's no documentation at all (nor any official examples) that show this most basic of cases. I understand that this is a WIP and that the readme was most likely written by developers rather than a technical doc writer, but that's no excuse for picking such an unhelpful example case.

Edit: Man, that was fast! Thanks for the update!

@isochronous
Copy link

I edited my previous comment, but since that won't trigger any notifications, I'll copy what I added here: Thanks for the quick update with the much more useful event handler example!

@jeremyrader
Copy link

@TimvdLippe not anymore, it is @ now, like @change=${...}

Had to do quite a bit of digging to find this. Thanks for the clarification. Would love to see an example for this in the docs.

@thepassle
Copy link

thepassle commented Nov 4, 2018

@jeremyrader The docs are work in progress, and coming soon. If you're looking for some code examples maybe this is helpful to you.

@ghost ghost changed the title Best practices for adding event listeners to a host element [docs] Best practices for adding event listeners to a host element Dec 13, 2018
@ghost ghost mentioned this issue Dec 15, 2018
2 tasks
@bennypowers
Copy link
Contributor

If you need to imperatively add a listener to rendered children, it must be added in firstUpdated or later, IIRC.

@ghost ghost closed this as completed in #413 Feb 5, 2019
LeonEck added a commit to LeonEck/eck-autocomplete that referenced this issue Mar 4, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.