Closed royriojas closed 8 years ago
Sorry it took so long to respond. The PRs are truly appreciated. This would create an API inconsistency with proxyquire which we'd like to avoid. However, if we can get proxyquire-universal doing this automatically, I think we can do this as only a minor bump. Are you up for taking a crack at that? I should have time this weekend to help substantially.
hi @bendrucker,
Why would this cause an API inconsistency, I was under the impression that it was a feature of proxyquire
, meaning allowing to mock require calls inside methods. Maybe we can find a solution that allows us to do it without creating any inconsistency in the API, if possible. I can help if you want. Just let me know how, happy to help you guys.
proxyquire.reset
would be in proxyquireify but not proxyquire
Hey @royriojas, sorry to let this go cold. In order to move this forward, I want to get a test case written that passes in proxyquire in Node but fails via proxyquireify. If you're up for taking the lead on this, I'm happy to provide guidance.
yes, please provide some guidance. Should I add the same test in proxyquire and see it failing with proxyquireify right?
Rather than opening a PR w/ proxyquire, I'd like you do to a separate repo with a failing test. You can use proxyquire-universal and basically make the test script do the following via your package:
{
"test": "npm run test-node && npm run test-browserify",
"test-node": "node test.js",
"test-browserify": "browserify -p proxyquire-universal test.js | node"
}
The goal here is to end up with a single file that behaves differently in proxyquire vs proxyquireify (i.e. exits 1 w/ proxyquireify and 0 with browserify).
Once the problem is documented/verified/reproducible, we start over on this PR with a failing test and provide coverage against your reset
method.
Then there's the issue of documenting this. Our general position is that requiring inline is bad. It needs to be clear that you rarely need to use reset
except for special cases. Of course the ideal approach would be eliminating the API difference between proxyquireify and proxyquire, but I'm not certain that's possible.
Also, no hard feelings if you don't have the time to go about working through a thorough solution to this right now. I don't think we'd be comfortable merging as is but I understand I'm asking for a lot above.
It is ok @bendrucker, I don't fully understand why require inside methods is considered a bad thing. But with ES6 modules now having the same limitation it might make sense to avoid it.
So to be clear we need a separate github repo only with the test that fails in proxyquireify
but does not in proxyquire
right?
why require inside methods is considered a bad thing
There is rarely a good reason to do it, except to avoid circular dep issues. Even there it's the wrong solution to the problem.
So to be clear we need a separate github repo only with the test that fails in proxyquireify but does not in proxyquire right?
Exactly. Separate repo that can easily be run in raw Node vs. browserify, former passes, latter fails.
Seems like this is going to be too challenging to get right and document well. For now I'd like to stick with proxyquireify not supporting inline requires.
NOTE: stub and cache need to manually be cleared.