injected array instance fails test for instanceof Array #13

Closed
aulphar opened this Issue Sep 10, 2012 · 8 comments

5 participants

@aulphar

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
Collaborator

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

@felixge
Owner

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
Collaborator

@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
Owner

@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
Owner

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

@domenic
Collaborator

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 :)

@domenic domenic closed this in 83f1d27 Feb 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment