Memory leaks when using newInstance to create multiple hljs instances

[immccn123]

Describe the issue/behavior that seems buggy

Calling newInstance multiple times poses a risk of memory leaks.

Sample Code or Instructions to Reproduce

see remarkjs/react-markdown#791 (comment)

Expected behavior

newInstance should not call window.addEventListener('DOMContentLoaded', boot, false).

Additional context

In function HLJS:

highlight.js/src/highlight.js

Lines 838 to 840 in e1fa2ea

	if (typeof window !== 'undefined' && window.addEventListener) {
	window.addEventListener('DOMContentLoaded', boot, false);
	}

Related:

remarkjs/react-markdown#791 rehypejs/rehype-highlight#31

[joshgoebel]

I can see the issue here, though newInstance is not really intended to be used more than a few times (for custom usage scenarios) - there is a high startup cost if you are using lots grammars. React should not be calling it for say, every render loop. Instead some parent component should likely hold onto a single instance and use that for most rendering.

[joshgoebel]

How would anyone suggest we fix this, track some global state on window itself, or?

[immccn123]

I don't think a standalone hljs instance should have global side effects. Perhaps an additional configuration option would be better, but I'm not sure if this would break highlight.js and it might increase the bundle size.

It seems like that the operations on the window object are only these.

[joshgoebel]

What if this code was moved inside the conditional portion of highlightAll() such that it's only called if one requests highlighting of everything?

[immccn123]

What if this code was moved inside the conditional portion of highlightAll() such that it's only called if one requests highlighting of everything?

Seems like a good idea

[erha19]

@immccn123 is there any new version for this fix?

[callie-openai]

@joshgoebel @immccn123 Also curious about this, saw the updated CHANGES.md for 11.11.0 but no associated publish to npm yet.

[joshgoebel]

Will try to get a few more of the open PRs resolved and then do a release after that. Wanting the next 11 release to be the last and then switch over to v12 betas.