instanceof operator considered harmful

[kumavis]

The instanceof operator is a nice and easy way of doing class detection in javascript in a single project. However side effects creep in when we look at this at the ecosystem level.

A project composed of three.js-based libraries will run into difficulties doing class detection. This is because they may have their own copies of the classes. One instance of Object3D may fail the instanceof test against the other module's Object3D class.

There are a number of issues originating from the use of this pattern:

• instanceof and architecture for extension #5673
• instanceof with multiple windows/iframes #5886

It would also be a requirement for the proposed module-focused architecture change:

• Moving to a modular architecture #4776

The primary solution is to moving to a type detection via feature detection as proposed by @substack.

The usage would look something like this:

ExampleClass.instanceof( obj )

It should return true if the obj appears to be an instance of the class or a subclass. It will perform the detection by checking for the presence of properties and methods on the obj associated with the class. We can still use instanceof to short circuit and fallback to feature detection for performance.

ExampleClass.instanceof = function(obj){
  return (obj instanceof ExampleClass) || ( x in obj && y in obj )
}

This would become part of the standard three.js class definition pattern.

Please suggest any other approaches that could work.

[ghost]

@kumavis Is there any situation you can think of where instanceof or any similar mechanism is strictly necessary? I think checking what type of class something is is an antipattern. I think it would be better to focus on eliminating its use altogether.

[christopherjbaker]

While most of the uses of instance of are for class-specific code, which belongs in the specific class, as noted in the browserify issue, instanceof works perfectly fine in all cases I have used it (ignoring the case of sharing objects between frames, as this is wrong).

If you want to add a thing to the threejs world, it needs to extend THREE.Object3D, just like everything else. If you want to create a custom mesh, it should extend THREE.Mesh. If you are creating a custom cube camera with slightly different functionality, it should extend THREE.CubeCamera (all of these are my own personal use cases).

An instance (foo) of the following will pass all of these: foo instanceof Foo, foo instanceof THREE.Mesh, foo instanceof THREE.Object3D

var Foo = function() {
    var geometry = ...;
    var material = ....
    THREE.Mesh.call(this, geometry, material);
}

Foo.prototype = Object.create(THREE.Mesh.prototype);

Foo.prototype.clone = function() {
    THREE.Mesh,prototype.clone.call(this);
    // custom logic
};

[kumavis]

@coballast thats a good point, and I don't have a good example of when we need to do class checking.

[kumavis]

@HMUDesign so the case I'm imagining is if you have the following dep graph

your_app
  threeRev65
  depA
    threeRev60
  depB
    threeRev61

Both depA and depB produce instances of Object3D subclasses. If there is class detection going on then it will fail.

One way to solve this is passing the three.js global down to the deps, but thats lame and breaks the point of a nested dep graph.

[makc]

this seems to be heavily related to #6241

[kumavis]

@makc to clarify, this is not a necessary change for #6241, but it is related to the principles of modularity.

Abstraction is the greatest tool we have in programming -- the framework approach puts an endcap to abstraction, whereas the modular approach allows us to build even taller pyramids, to stand on the shoulders of even larger giants.

[thure]

+1 – this is an excellent perspective.

[Quasimondo]

I am currently observing an issue related to instanceof which I find strange: in WebGLRenderer.renderObjects() I can observe in the debugger that in this part:

     if ( buffer instanceof THREE.BufferGeometry ) {
         _this.renderBufferDirect( camera, lights, fog, material, buffer, object );
     } else {
          _this.renderBuffer( camera, lights, fog, material, buffer, object );
   }

the check does not always work. I see objects which have a .type="BufferGeometry" that are not recognized by instanceof THREE.BufferGeometry and the code executes .renderBuffer() instead of renderBufferDirect(). In the same test other BufferGeometry objects are recognized. All the geometries in this case had been created using
mesh.geometry = new THREE.BufferGeometry().fromGeometry( mesh.geometry );

[makc]

how is that possible? fromGeometry sits in BufferGeometry.prototype and returns this.

[mrdoob]

@Quasimondo Oh, that's weird. Any chance you could create a jsfiddle that reproduces the issue?

[dubejf]

It can happen if you include three.js twice.

Modify the getting started example to include <script src="js/three.min.js" async></script> to reproduce.

[mrdoob]

Ohm... Whyyyyyy

[dubejf]

1. Library is loaded.
2. Some objects are created.
3. Library is loaded again, replacing the old instances.
4. The prototypes of objects created at step 2 still point to the old instances. instanceof fails because the prototypes have the same name but are not the same instances.

Basically this:

window.MyObject = function () {};
var v1 = new window.MyObject();
console.log( 'first object instanceof:', v1 instanceof window.MyObject ); // true

window.MyObject = function () {};
var v2 = new window.MyObject();
console.log( 'second object instanceof:', v2 instanceof window.MyObject ); // true
console.log( 'first object instanceof:', v1 instanceof window.MyObject ); // false!

[mrdoob]

Interesting... I wish Javascript had some sort of return in global scope.

if ( THREE !== undefined ) return;

[kumavis]

if ( THREE !== undefined ) throw new Error('THREE already defined -- carry on');

[kumavis]

@dubejf I guess I don't understand why you're loading three.js twice though...

[mrdoob]

if ( THREE !== undefined ) throw new Error('THREE already defined -- carry on');

Oh! that works? We should add it then.

[makc]

It can happen if you include three.js twice.

I mean, how does someone even think of that?

https://twitter.com/sempf/status/514473420277694465

[Quasimondo]

Oh well - I'm very sorry but it looks like now I cannot recreate the conditions under which it happened. Now it works as expected again.

[dubejf]

@kumavis To clarify, I'm not loading three.js twice, just trying to guess what might be the cause of @Quasimondo's error - although I've seen similar errors before while experimenting with lazy loading.

Another possibility to explain instanceof failure is if <frame> are involved.

I wish Javascript had some sort of return in global scope

One way to prevent the script from executing twice would be to wrap the content of three.js in a function and return with a warning if the library is already defined, aka the Module pattern.

(function(window){

    if ( window.THREE !== undefined ) {

        // warning
        return;

    }

    var THREE = { REVISION: '71' };
    ... 

})(this);

[Quasimondo]

I am pretty sure that I am not loading three.js twice. And no iframes in use either. The setup where I observed that behavior was a combination of loading a Collada scene, merging several geometries and then converting them to BufferGeometry.

[Quasimondo]

I just could reproduce the situation and it turns out that it's a glitch in the Firefox debugger. As you can see in the screenshot I set a breakpoint at the ´_this.renderBuffer(´ line - which should not be called if buffer is instanceof BufferGeometry, but FF stopped there. Setting a breakpoint inside renderBuffer itself though does not trigger a break, so it is actually never called. Sorry for the fuss.

[kumavis]

@dubejf you're right about the closure, thats the best way to handle early top-level returns (and also avoid accidently implicit creating some globals).

[kumavis]

@Quasimondo thats a weird one -- if it turns out to be actually related to three.js we can reopen this issue.