fix for OSD 'unload' before is has time to bootstrap

[edsilv]

Hi,

I'm building an app where instances of OSD need to be able to be created/replaced in quick succession.

A simplified example would be if you had two buttons on a page. Whenever Button A is clicked OSD is used to load an image, whenever Button B is clicked a 3D model is loaded in Google model-viewer.

If both of these viewers inhabit the same container div whose contents are being emptied, and you click between Button A and Button B rapidly, you occasionally get an error here where it's trying to append to a null this.element reference.

I've simply introduced a check to see if this.element exists when bootstrapping OSD and returning if it doesn't. This fixed the issue for me.

Thanks for all the great work!

[edsilv]

Interestingly, Google model-viewer does not seem to exhibit a similar issue when being replaced while "bootstrapping". (i.e. its container div being emptied and something else being put in its place). I'd like to avoid a special case of calling destroy on OSD if possible.

[edsilv]

In the meantime I've disabled the buttons to switch between viewers while OSD is loading a tilesource. This fixes my use case, but does mean the UI is a little less "responsive".

[iangilman]

Thank you for filing this… We certainly want to be more robust if we can! I have some questions, though…

It doesn't seem like this fix would address anything to do with loading a tile source… The guard you've added is in the synchronous creation code, but loading a tile source is an asynchronous process. Can you explain more what's causing this issue to happen?

Trying to create a viewer without a proper element certainly is an error… I don't think we should be making that error invisible. We could add a console.error, and then the developer could see it, but it would also be impossible to detect by the code, if it wanted to. I suppose we could add some sort of "I didn't get created properly" flag to the viewer for that purpose. Throwing an exception manages to do all of that, though, so maybe it's already good as is, and if you want to suppress the error, you could wrap the viewer creation in a try/catch?

If the problem isn't in the creation itself but in the tile load sequence, I'm curious to know where it's falling down, and whether calling destroy on the viewer cleans that up. It does seem like calling destroy would be a good idea; why do you not want to do so?

[edsilv]

Hi Ian,
I'm on another project now for a couple of weeks, but will be revisting this. I'll try to create a simplified test harness to demonstrate what's happening then. I ran into the same issue elsewhere in my react app as well.
The app is ultimately going to be able to load a variety of content types, and I was hoping to avoid making a special case for OSD. Perhaps OSD isn't automatically calling destroy internally in certain circumstances? e.g. a react wrapper component being unmounted?

[msalsbery]

@edsilv I'm not sure we currently need a check like this...the containing element needs to exist at the time a OpenSeadragon.Viewer is created, and this check could easily be done outside the library right before calling new OpenSeadragon.Viewer(...).

Also, I don't believe Viewer.destroy() is called implicitly, so it's important that's explicitly handled where appropriate.

It's important to make sure the element or element ID passed to the Viewer constructor exists. If the real-life case is OpenSeadragon in a React component, when the constructor gets called in the React component tree lifecycle is important. For what it's worth, here's what I typically use. Note I have the containing element in the component so it always exists when the component mounts...

import React, { useEffect } from 'react';
import './OsdViewer.scss';

import OpenSeadragon from 'openseadragon';

import osdNavImages from '../common/OsdNavImages';

const tileSourcesPrefix = '/dzimages/';
const tileSources = [tileSourcesPrefix + 'testpattern.dzi'];

function OsdViewer(props) {
	useEffect(() => {
		let viewer = new OpenSeadragon.Viewer({
			debugMode: false,
			id: 'osdContainer',
			prefixUrl: '',
			navImages: osdNavImages,
			useCanvas: true,
			tileSources: tileSources
		});

		let onOpen = function (event) {};

		let onClose = function (event) {};

		viewer.addHandler('open', onOpen);
		viewer.addHandler('close', onClose);

		// Cleanup (componentWillUnmount)
		return () => {
			// < Cleanup any plugins that may have references to viewer here >
			viewer.removeHandler('open', onOpen);
			viewer.removeHandler('close', onClose);
			viewer.destroy();
			viewer = null;
		};
	}, []);

	return <div id="osdContainer" className="openseadragon" />;
}

export default OsdViewer;

When you come back to this issue I'd really like to make sure we don't have to fix anything in OpenSeadragon for this, and if we do, we get it fixed ;)