Undefined check for object descriptor

[joedf]

The new fix #2201 has broken my OpenSeadragon application. I realized we should have an 'undefined" check for Object.getOwnPropertyDescriptor(). This fixes the issue and logs a warning when it is undefined.

A property descriptor of the given property if it exists on the object, undefined otherwise.

I get the warning for removeItem in my application. I'll have to look into it on my own at some point, but at least this can go in for now. It looks like name = "removeItem" and options is an array of length 0...

Perhaps, there should be a check for options.length = 0?

Comments?

[iangilman]

Interesting! So this must be when one of the objects passed into extend has properties that are not its own (i.e. inherited)? I suppose we need to think about what we would want in that case. My initial intuition is we don't necessarily want to include properties that are inherited.

How are you getting this to happen? The only place in OSD I'm aware of there being a removeItem is the World.prototype, and we're never using that to extend another object. Is that the one? Are you doing something special with it?

[joedf]

I didn't think I was... I also have removeItem on some of my own objects, but they are not related to OpenSeadragon... at least not on page load. The warnings only occur during Viewer creation... var Viewer = OpenSeadragon({ ... options here ]);

Not sure if this helps, but I am using the js-inheritance script from John Resig, and additionally jQuery. The reason will still do is that the application pretty old... it is maintained by me, but haven't change some dependencies because we had the requirement to support IE11, but that should be no longer soon enough.

[iangilman]

Hmm... I'm not getting it when I run the basic test (https://github.com/openseadragon/openseadragon/blob/master/test/demo/basic.html), so there must be something different in how you're running it. Are you subclassing any of the OSD classes? It seems like it might be worth knowing what's causing this before we merge the fix.

[joedf]

Not sure, the only "weird" and not best practice stuff I can think of would be overriding:

• viewport.applyConstraints = function( ...
• and taking control of some mouse event handlers:

this.osdMouseTracker = new OpenSeadragon.MouseTracker({
	element: this.canvas,
	releaseHandler: this.osdViewer.innerTracker.releaseHandler,
	dragHandler: this.osdViewer.innerTracker.dragHandler,
	dragEndHandler: this.osdViewer.innerTracker.dragEndHandler,
	scrollHandler: this.osdViewer.innerTracker.scrollHandler
});
this.osdMouseTracker.setTracking(true);

[iangilman]

Great, this seems like a good fix.

I guess the existence of this warning means you'll be getting it in your app all the time. Are you okay with that?

[joedf]

Yeah, I think that's okay. If not, I can always locally build and remove the warning for my own use if I find it too noisy in the console. Thanks!

[iangilman]

Sounds fair. Let's go with this. Thank you for the fix!