thlorenz / proxyquireify

browserify >= v2 version of proxyquire. Mocks out browserify's require to allow stubbing out dependencies while testing.
MIT License
152 stars 24 forks source link

Calls to `require` inside methods get the original dependency instead of the mocked one #40

Closed royriojas closed 8 years ago

royriojas commented 9 years ago

Given the following files

/********************/
//bar.js
function bar () {
  return 'bar';
}

function rab () {
  return 'rab';
}

module.exports = { bar : bar, rab: rab };
/********************/
// foo.js
module.exports = {
  create: function () {
    return {
      bar: function () {
        return require('./bar').bar(); // this call is not mocked
      }
    };
  }
};

then

var proxyquire = require('proxyquireify')(require);

var foo = proxyquire('./foo.js', { 'bar': { bar: function () { return 'barber' } }}).create();

foo.bar() // expected: 'barber', actual: 'bar'

It seems the cache is delete as soon the proxyquire call ends. I have made a quick fix on my fork, for this but I don't know if there is any side effect (apart from making the users manually having to call reset to clear both the dep cache and module cache.)

Do you see any particular issue with a fix like this one?

https://github.com/royriojas/proxyquireify/commit/77049f1fe33cecba08d7c0f252a0157e550033e3

thlorenz commented 9 years ago

Please provide this as a PR and we can discuss. In general we are trying to get/stay on par with the API and behavior of thlorenz/proxyquire.

royriojas commented 9 years ago

@thlorenz proxquire does not proxy calls inside methods? if not, then you're right and this is not required.

thlorenz commented 9 years ago

proxyquire does proxy require calls inside methods, see this test and related fixture.

If proxyquireify isn't doing this currently then we should fix it, even though it is low priority as people should require all their dependencies on top of the module with few exceptions.

royriojas commented 9 years ago

@thlorenz created this pull request #41 with the test case as well. Let me know if it makes sense

bendrucker commented 8 years ago

Let's continue the discussion in #41 on this. No need to have an issue + PR open for discussion.