Resolve the memory leak problem when creating multiple HLJS instances (#4086) #4095
Conversation
src/highlight.js
Outdated
| function boot() { | ||
| // if a highlight was requested before DOM was loaded, do now | ||
| highlightAll(); | ||
| window.removeEventListener('DOMContentLoaded', boot, false); |
There was a problem hiding this comment.
What is the thinking here? Is this type of cleanup truly necessary?
There was a problem hiding this comment.
This cleanup might only matter in rare cases, like when an hljs instance is used just once and highlightAll is called before DOMContentLoaded. In such a scenario, the instance would stay in memory, unable to be released. If this corner case isn't a concern, the cleanup is likely unnecessary.
If this contradicts the core philosophy, the cleanup can be removed.
There was a problem hiding this comment.
I'm just going to call it unnecessary -- unless you have some docs (from MDN or such) that says this is somehow best practice... I've seen tons of onload hooks like this over the years but never code to unwind a single-use event listner afterwards - seems a browser concern... if the browser knows that event that just fired can never be fired again on the same page it could do the cleanup itself.
There was a problem hiding this comment.
I understand your point. While the event could technically be triggered using window.dispatchEvent(new Event("DOMContentLoaded")), which isn't common practice, this means the browser wouldn’t automatically handle the cleanup. However, another approach to ensure the listener is only triggered once and automatically cleaned up is to use window.addEventListener(..., { once: true }). Is this one better, or just remove the cleanup?
Would you prefer switching to { once: true }, or should we remove the cleanup entirely?
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/API/Document/DOMContentLoaded_event
I find zero examples of once being suggested. Lets just remove the cleanup.
There was a problem hiding this comment.
Thanks for the clarification and patience! I've removed the cleanup.
|
Awesome, now can you just add an entry to CHANGES and then I think this is good to merge. |
Build Size ReportChanges to minified artifacts in 3 files changedTotal change -44 B View Changes
|
|
Changelog entry has been added as requested. Thanks for your guidance throughout this process! |
Build Size ReportChanges to minified artifacts in 3 files changedTotal change -45 B View Changes
|
|
@immccn123 hi, is there any new version for this fix? |
Fixes the memory leak problem mentioned in the above issue, using #4086 (comment) as the solution.
Resolves #4086.
Changes
The event listener is added to the window only when
highlightAll()is called, and the listener will be removed by itself when DOM content is loaded.Checklist
CHANGES.md