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.

Sign up for GitHub

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

Resolve the memory leak problem when creating multiple HLJS instances (#4086) #4095

Merged
merged 3 commits into from
Aug 22, 2024
+14 −10
Merged

Resolve the memory leak problem when creating multiple HLJS instances (#4086) #4095

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c64e08
fix: The memory leaking issue when creating multiple HLJS instances
immccn123 Aug 19, 2024
3439760
remove unnecessary cleanup
immccn123 Aug 21, 2024
918861e
update changelog
immccn123 Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/highlight.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,8 +819,18 @@ const HLJS = function(hljs) {
* auto-highlights all pre>code elements on the page
*/
function highlightAll() {
function boot() {
// if a highlight was requested before DOM was loaded, do now
highlightAll();
window.removeEventListener('DOMContentLoaded', boot, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the thinking here? Is this type of cleanup truly necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification and patience! I've removed the cleanup.

}

// if we are called too early in the loading process
if (document.readyState === "loading") {
// make sure the event listener is only added once
if (!wantsHighlight) {
window.addEventListener('DOMContentLoaded', boot, false);
}
wantsHighlight = true;
return;
}
Expand All @@ -829,16 +839,6 @@ const HLJS = function(hljs) {
blocks.forEach(highlightElement);
}

function boot() {
// if a highlight was requested before DOM was loaded, do now
if (wantsHighlight) highlightAll();
}

// make sure we are in the browser environment
if (typeof window !== 'undefined' && window.addEventListener) {
window.addEventListener('DOMContentLoaded', boot, false);
}

/**
* Register a language grammar module
*
Expand Down