Conflict with menu library

[gjcope]

An issue was reported with the component crashing when used on a page with the mmenu library. This looks to be due to that library manipulating the DOM which results in the Voyager component being disconnected and reconnected somewhere else while still loading.

Currently the main Explorer component disconnect callback disposes of the app on the (now incorrect) assumption that we are done with it if disconnected.

dpo-voyager/source/client/ui/explorer/MainView.ts

Line 173 in 894eadc

this.application.dispose();

Removing this solves the issue but potentially leaks if the intent when disconnecting is for the component to be completely removed.

@sdumetz Interested on your take here, I know you've done some memory management work.

[sdumetz]

As far as I know, this will all get freed on garbage-collection. We just need to make sure the element is garbage-collected when disconnected (which it won't be if we have leftover eventListeners).

Other than bad callbacks cleanup, memory would leak if the user inadvertently keeps a reference to the element or any of its children but that's expected.

You might want to document the call to explorer.application.dispose() as a way to explicitly clean things up.

They might still run into some unexpected trouble because the whole tree is designed under the assumption that it will have a linear connect->load->show->end flow. I know this because that's the kind of things we do in our holographic(ish) scenes (moving elements around, dynamically removing and attaching individual UI elements) and it's not very stable.

This unconventional usage of voyager is probably the root cause of my still-a-bit-mysterious memory leak, for example.

So from my point of view, this change would be a good thing if we ensure proper GC afterwards.

[sdumetz]

I did some tests.

With the current behaviour, memory (most of it anyway) is freed during the next GC run (not sooner, because it is not possible to manually release an ArrayBuffer). However voyager-explorer itself is never garbage-collected.

Best way I found to quick-check if a handle gets GC-ed is (in the console):

registry = new FinalizationRegistry(message => console.log(message));
(()=>{
  let item = document.querySelector("voyager-explorer");
  item.parentNode.removeChild(item);
  registry.register(item, "EXPLORER GC");
})();

It's actually quite hard to touch things in the console without actually holding a reference to them.

The thing is, because anything that inherits from Component holds a reference to the whole System and Graph, anything that doesn't get properly disconnected will prevent the whole stack from being collected.

Just checking things within voyager-explorer's MainView, we have Notification.shadowRootNode and this.addEventListener('focus', this.onFocus); that are keeping a ref and need to be moved to connect() and unassigned in disconnect.

Then we need to move down the tree to everything that forgets to remove its references or event callbacks.

That's long and boring but not particularly difficult. I might be able to do this at some point (as I said, I'd like to be able to use Voyager more as a "portable library of custom elements" and this kind of changes is required for this to work reliably).

But as I understand it, this would yield 0 benefit for your (and most of mine) use cases. Is this an intended use of voyager for the Smithsonian? Are you interested in doing this or not particularly?

[gjcope]

Thanks for this, this is really helpful and a clear path to follow.

But as I understand it, this would yield 0 benefit for your (and most of mine) use cases. Is this an intended use of voyager for the Smithsonian? Are you interested in doing this or not particularly?

That's correct, it would not impact the majority of our use cases and I wouldn't call it an intended use case. But I don't really like the fact that a random external library can crash the component so I'd like to improve the robustness where possible. Definitely not a priority but I'll leave this issue open for when I have the time to walk through the references.

Thanks again for the insights!