-
Notifications
You must be signed in to change notification settings - Fork 168
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
Add section on avoiding memory leaks to events.md #1240
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is a great thing to point out. I do wonder if we should provide an alternative for the presented bad code but I'm not sure what the best fix for that would be... Maybe to add the event listener in connectedCallback
and add the condition inside the handler callback?
@@ -98,6 +98,32 @@ disconnectedCallback() { | |||
|
|||
See the MDN documentation on using custom elements [lifecycle callbacks](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#Using_the_lifecycle_callbacks) for more information on `connectedCallback` and `disconnectedCallback`. | |||
|
|||
### Avoiding Memory Leaks | |||
|
|||
Avoid mixing and matching adding and removing event listeners between the [Lit reactive lifecycle methods](/docs/components/lifecycle/#reactive-update-cycle) and custom element [lifecycle callbacks](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#Using_the_lifecycle_callbacks). These two lifecycles, in some cases, operate independently of one another. For example, just because `disconnectedCallback` has been called does not mean that `shouldUpdate`, `willUpdate`, `update`, `render`, and `updated` will not be called again if a [reactive property](/docs/components/properties/) is changed after the element is removed from the DOM. This can result in memory leaks when listeners attached to active objects contain back references to the component that set them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be blasphemous but we do a single space after periods.
Avoid mixing and matching adding and removing event listeners between the [Lit reactive lifecycle methods](/docs/components/lifecycle/#reactive-update-cycle) and custom element [lifecycle callbacks](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#Using_the_lifecycle_callbacks). These two lifecycles, in some cases, operate independently of one another. For example, just because `disconnectedCallback` has been called does not mean that `shouldUpdate`, `willUpdate`, `update`, `render`, and `updated` will not be called again if a [reactive property](/docs/components/properties/) is changed after the element is removed from the DOM. This can result in memory leaks when listeners attached to active objects contain back references to the component that set them. | |
Avoid mixing and matching adding and removing event listeners between the [Lit reactive lifecycle methods](/docs/components/lifecycle/#reactive-update-cycle) and custom element [lifecycle callbacks](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#Using_the_lifecycle_callbacks). These two lifecycles, in some cases, operate independently of one another. For example, just because `disconnectedCallback` has been called does not mean that `shouldUpdate`, `willUpdate`, `update`, `render`, and `updated` will not be called again if a [reactive property](/docs/components/properties/) is changed after the element is removed from the DOM. This can result in memory leaks when listeners attached to active objects contain back references to the component that set them. |
connectedCallback () { | ||
super.connectedCallback(); | ||
this.listeningForResize = false; | ||
} | ||
disconnectedCallback() { | ||
window.removeEventListener("resize", this.handleWindowResize); | ||
super.disconnectedCallback(); | ||
} | ||
handleWindowResize = (event) => { | ||
this.style.width = "880px"; | ||
} | ||
willUpdate(changedProperties) { | ||
if (!this.listeningForResize) { | ||
window.addEventListener("resize", this.handleWindowResize); | ||
this.listeningForResize = true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit around code style – single quotes for strings and underscore for private method and unused param. Also inline comment to make it clear this shouldn't be done.
connectedCallback () { | |
super.connectedCallback(); | |
this.listeningForResize = false; | |
} | |
disconnectedCallback() { | |
window.removeEventListener("resize", this.handleWindowResize); | |
super.disconnectedCallback(); | |
} | |
handleWindowResize = (event) => { | |
this.style.width = "880px"; | |
} | |
willUpdate(changedProperties) { | |
if (!this.listeningForResize) { | |
window.addEventListener("resize", this.handleWindowResize); | |
this.listeningForResize = true; | |
} | |
} | |
connectedCallback () { | |
super.connectedCallback(); | |
this.listeningForResize = false; | |
} | |
disconnectedCallback() { | |
window.removeEventListener('resize', this._handleWindowResize); | |
super.disconnectedCallback(); | |
} | |
_handleWindowResize = (_event) => { | |
this.style.width = '880px'; | |
} | |
willUpdate(_changedProperties) { | |
if (!this.listeningForResize) { | |
// This might never get removed and cause a memory leak! | |
window.addEventListener('resize', this._handleWindowResize); | |
this.listeningForResize = true; | |
} | |
} |
@@ -98,6 +98,32 @@ disconnectedCallback() { | |||
|
|||
See the MDN documentation on using custom elements [lifecycle callbacks](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#Using_the_lifecycle_callbacks) for more information on `connectedCallback` and `disconnectedCallback`. | |||
|
|||
### Avoiding Memory Leaks | |||
|
|||
Avoid mixing and matching adding and removing event listeners between the [Lit reactive lifecycle methods](/docs/components/lifecycle/#reactive-update-cycle) and custom element [lifecycle callbacks](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements#Using_the_lifecycle_callbacks). These two lifecycles, in some cases, operate independently of one another. For example, just because `disconnectedCallback` has been called does not mean that `shouldUpdate`, `willUpdate`, `update`, `render`, and `updated` will not be called again if a [reactive property](/docs/components/properties/) is changed after the element is removed from the DOM. This can result in memory leaks when listeners attached to active objects contain back references to the component that set them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see a description of the problematic structure so that readers can develop a mental model of when event listeners will hold on to the element.
Something like:
Event listeners can sometimes cause memory leaks if they're not removed, depending on the objects or elements they're added to and the objects they reference in the body of the event handler.
Listener functions hold a JavaScript reference to variables closed over in the function body. This behaves as if you added a property to the event target that references the objects used in the handler.
When this can often cause a leak is when a listener is added to an object outside of the custom element or it's children, that references back to the element:
class LeakyElement extends LitElement {
constructor() {
super();
// This will cause LeakyElement instances to never be GC'ed if not removed:
window.addEventListener('mousemove', this.#onMouseMove);
}
#onMouseMove = (e) => { /* ... */ };
}
To prevent a memory leak, you should added these listeners in connectedCallback()
and remove them in disconnectedCallback()
:
class LeakyElement extends LitElement {
connectedCallback() {
super.connectedCallback();
window.addEventListener('mousemove', this.#onMouseMove);
}
disconnectedCallback() {
super.disconnectedCallback();
window.removeEventListener('mousemove', this.#onMouseMove);
}
#onMouseMove = (e) => { /* ... */ };
}
Note that the reference to the event listener needs to be the same in the addEventListener()
and removeEventListener()
calls. Using a class field initialized to an arrow function is a convenient way to do this.
Event listeners added by an element to itself or a child do not usually cause a leak because they create a cycle that GCs can collect:
class LeakyElement extends LitElement {
constructor() {
super();
// This will *not* cause a leak:
this.addEventListener('click', this.#onClick);
}
#onClick = (e) => { /* ... */ };
}
One of the most common causes of detached HTML Elements remaining in memory after a developer expects them to be removed and cleaned up is attributed to them not realizing that the Lit reactive lifecycle methods can be called again after
disconnectedCallback
. I think it might be prudent to add a section to the documentation that calls out the issue and gives a warning. This is my first attempt, if edits are needed please advise.