Possible memory leak (not cleaning up preact root nodes)?

[genadis]

haven't tested it, just pointing it here to take notice.

Steps are rendered using preact 'render' which creates preact root nodes: https://github.com/shipshapecode/shepherd/blob/master/src/js/step.jsx#L237

when steps are destroyed: https://github.com/shipshapecode/shepherd/blob/master/src/js/step.jsx#L156
a simple this.el.parentNode.removeChild(this.el); is used.

according to:
preactjs/preact#1151
preactjs/preact#53

Destroying preact root node should be something like: render('', parent, root); or render(null, parent, root);
and there was a suggested wrapper for that: preactjs/preact#53 (comment)

same goes for the modal component: https://github.com/shipshapecode/shepherd/blob/master/src/js/tour.jsx#L90
when the tour constructor is executed it is assumed that if there were previous tour the modal will still be attached to the dom document.querySelector(.${this.classPrefix}shepherd-modal-overlay-container);
If options.modalContainer is used that might not be the case (options.modalContainer might be detached from DOM by app and reattached at later stage causing 2 modals).
But I guess regarding modal it's debatable what it the right approach. would be nice though to have an api method to destroy the modal of completed tour, even if it's manual to be done by the using app.

[RobbieTheWagner]

@genadis I'm curious, if you have not tested this, what has indicated there is a problem? If we have a memory leak, we should definitely fix it, but we should start with a failing test showing something is not cleaned up, then fix it by correctly cleaning up.

[RobbieTheWagner]

@genadis I opened a PR doing what you suggested, but I would like a way to definitively test this. Since this.el.parentNode is the tippy element, and that gets destroyed, I'm not sure if we really even need to remove anything at all.

I would love to see some definitive results of one approach versus the other.

[RobbieTheWagner]

I'm going to close this for now. If you have a concrete case showing a memory leak, let me know and we can take a look again.