injected array instance fails test for instanceof Array

[ghost]

I found a defect in latest version that when i inject an array and it will not be an instance of Array any more, reproduce with below code

// lib.js
module.exports = {
    'a' : 1,
    'b' : [1,2,3]
}


// target.js
var lib = require('./lib')
    , assert = require('chai').assert

module.exports.test = function(){
    return lib.a
}

module.exports.test2 = function(){
    console.log(lib.b)
    console.log(lib.b instanceof Array)
        // should be true
    return lib.b instanceof Array
}

// test.js, with mocha 
describe('SandboxedModule', function() {
  it('should return true, for an array', function() {
    var target = SandboxedModule.require('./helper/target', {
      requires : {
        './lib' : {
          'b' : [9, 8, 7]
        }
      }
    })
    assert.isTrue(target.test2());
  })
});

[cliffano]

I'm having the same problem.

The weird thing I found is that, for x being an array used in a sandboxed-injected module

• x instanceof Array -> false
• x.constructor === Array -> false
  but
• Array.isArray(x) -> true

I created a repo to demonstrate this problem at https://github.com/cliffano/sandboxed-constructor-demo

To compare the values before and after being passed to a required function:
$ node a1.js
BEFORE Constructor check: true
BEFORE Instanceof check: true
BEFORE Array.isArray check: true
AFTER Constructor check: false
AFTER Instanceof check: false
AFTER Array.isArray check: true

And to show the values without sandboxed-module:
$ node a2.js
AFTER Constructor check: true
AFTER Instanceof check: true
AFTER Array.isArray check: true

Tested on node 0.4.7, 0.6.14, and 0.8.8.

I've verified that when the exports are created in requireInterceptor (sandboxed_module.js), exports([]) still gives true for all checks. I don't know what else could possibly change constructor property of the array.

@felixge , any hint on where to look?

Edit:
Moved the demo of the problem to https://gist.github.com/4152652

[danbell]

+1

[domenic]

+1, we have this problem with instanceof Error a lot since that is used by testing libraries.

[felixge]

So - off topic ... I currently don't use this module much anymore. If I really need to stub another module (for example, because it provides a class that is created by one of the methods of the module I'm testing), I do this:

In the module I'm testing:

ClassUnderTest.OtherClass = require('./OtherClass');

module.exports = ClassUnderTest;

function ClassUnderTest() {
  // ...
}

ClassUnderTest.prototype.someMethod = function() {
  var foo = new ClassUnderTest.OtherClass();
  foo.something();
}

In my test:

var ClassUnderTest = require('./ClassUnderTest');
ClassUnderTest.OtherClass = sinon.stub().returns(sinon.stub(new ClassUnderTest.OtherClass));

IMO that's much simpler than fucking with the module system.

That being said ... if anybody wants to maintain this module - I'd be happy to gibe out github / npm access! Any takers?

[domenic]

@felixge Yes, if you are using your own classes dependency injection is a nicer approach. (Whether it be property injection, as in your example, or constructor injection like I prefer.) But this falls down for modules like fs or http or similar, or on any third-party modules that aren't class-based.

---

Anyway... I might be up for helping maintain this package, as I've done for a few others, but I'm not well-versed in the code (i.e. I haven't submitted lots of pull requests) so I should not be your first choice. Hopefully someone else will come along that has spent time spelunking around in its innards.

[felixge]

@felixge Yes, if you are using your own classes dependency injection is a nicer approach. (Whether it be property injection, as in your example, or constructor injection like I prefer.

Well, I prefer constructor inject as well, but it doesn't work for internal collaborators (classes created within my class) like in my example. Well, one could inject the constructor I guess, maybe I'll switch to that style.

Anyway Domenic ... I'd love for you to help out with maintaining. If somebody else / better comes along, I can add them as collaborators as well, but for now I'll add you. All I want for you is the ability to scratch your own itch, that should already be great. And if you end up helping some other folks with theirs, even better!

Adding you to github/npm now.

[felixge]

@domenic done - you can now change anything you like - or not : ) !

[domenic]

Well, one could inject the constructor I guess, maybe I'll switch to that style.

Yeah that's what we do. Or factories that adapt the constructor by e.g. filling in some of its params but not all.

Thanks for adding me! I'll try to help out as much as I'm able :)

[chriswininger]

Don't know what their source is like, but I've observed the following frustrating behavior with node vm

function _test(grr) { console.log('!!! grr: ' + (grr instanceof Array)); }

_test([])
=> true

vm.runInNewContext('(function() { _test([]) })()', { console: console, _test: _test })
=> false

vm.runInNewContext('(function() { _test([]) })()', { console: console, _test: _test, Array: Array })
=> false

In this case Array.isArray will return true, just as mentioned here