Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can not get proxyquire to work with a require that happens in an init function #215

Closed
intervalia opened this issue Aug 17, 2018 · 13 comments · Fixed by #216
Closed

Can not get proxyquire to work with a require that happens in an init function #215

intervalia opened this issue Aug 17, 2018 · 13 comments · Fixed by #216

Comments

@intervalia
Copy link

I have some code like this:

/lib/mine.js

const path = require('path');

function init(cfg) {
  var inner = require(cfg.file);
  inner.doSomething();
}

module.exports = {init};

/app.js

const mine = require(('./lib/mine');
// Do some stuff here

const cfg = {
  file: '../otherPath/otherFile'
};

mine.init(cfg);

The code is much more complicated then this, but this is close enough to show the problem.

I am writing tests for app.js above and want to use proxyquire for the file ../otherPath/otherFile

My test file is like this:

app.mocha.js

const proxyquire = require('proxyquire').noCallThru();

const ofStub = {
  doSomething: function() {
    // Do something
  }
}

const app = proxyquire('./app', {
  '../otherPath/otherFile': ofStub
});

The rest of this file does not matter since the error happens when I try to use proxyquire on app

I get something like the following error:

Error: Cannot find module '../otherPath/otherFile

The thing is that it should not have tried to load that module yet since I have not called init

Is there a way to force proxyquire to not care if the real module was loaded using require when proxyquire is initially called and just allow the module to be overridden if and when it is loaded?

@bendrucker
Copy link
Collaborator

Hey, would you mind creating a simple repro project with this case? I think I understand what you're trying to do here but it'd be a big help to be able to see the error and debug it directly.

@intervalia
Copy link
Author

The failing code is in a private repo but I will create a public repo that demos the bug.

I will respond to this thread as soon as it is ready but I may not be able to get to it until Monday.

Thanks!!

@bendrucker
Copy link
Collaborator

Cool, thank you! I'll leave this open while you work on that.

@bendrucker
Copy link
Collaborator

An ideal repro project is setup such that we can npm install && npm test and get an exception.

@intervalia
Copy link
Author

OK. Maybe there isn't an error in your code. It looks like my code must be at fault.

Here is the repo I created so you can see what I was doing.
https://github.com/intervalia/proxyquire-215
It does pass tests. So I will dig further into my other code and let you know what I find.

But I really think it is my issue now and not yours.

@bendrucker
Copy link
Collaborator

😄 Thanks, that often happens with a repro, hopefully it's a helpful exercise!

@intervalia
Copy link
Author

intervalia commented Aug 20, 2018

I figured out what happened that made me think this was a bug.

In my tests I was using const proxyquire = require('proxyquire').noCallThru();

I had assumed that since I was telling proxyquire to NOT use the original required code that it would not bother trying to load the original file.

In my test code I was telling the system to require a non-existent file. And Proxyquire complained that the file didn't exist.

To me this felt off since I didn't need it to exist in my tests. I was overriding it through a stub after all.

So it may not be a real bug, but it felt like one since I said .noCallThru().

So maybe allow proxyquire to not bother checking to see if a file really exists when using .noCallThru() then my tests would not have failed.

Without change I now have to force the existence of a file, in my test environment, that is never used. If .noCallThru() didn't care about the existence of the required file then my code would have worked fine.

So maybe it is WAD (Working as Designed), but it seems like a nice option to ignore the existence of any file that is stubbed out when using .noCallThru().

If you guys want to mark this as WAD, that's fine, but maybe add some docs letting people know that every requireed file must exist even if you stub it out with .noCallThru() enabled.

OR: Maybe add an option into .noCallThru(false) that prevents the file exists code.

@intervalia intervalia reopened this Aug 20, 2018
@bendrucker
Copy link
Collaborator

This is the output I get:

____ load.js loaded
Stubbed success.js called


  proxyquire issue #215 tests
TESTING Lazy loading files from ../lazyload/one
____ load.js run
loading ../lazyload/one
running ../lazyload/one
Stubbed one.js called

Proxyquire only tries to load the original to provide call-thru functionality. Setting noCallThru() should eliminate any need to have the stubbed modules actually present.

Please do modify your example if I'm misunderstanding since as long as the tests are passing I expect it's working.

@intervalia
Copy link
Author

Sorry, I forgot to change my repo. It is now changed and shows the failure.

Again, this is a test only environment for me. I have a situation where I want to only stub out a file that does not exist. The code shows that. It used to require '../lazyload/one', which exists. Not it attempts to require '../lazyload/two' which does not exist but it 100% stubbed out in my test file (app.mocha.js).

@bendrucker
Copy link
Collaborator

Hmm, interesting. Looking into it.

@bendrucker bendrucker reopened this Aug 20, 2018
@bendrucker
Copy link
Collaborator

From some quick debugging I see that these errors are rethrown unless noPreserveCache() is on:

if (!this._preserveCache) {
return path.resolve(path.dirname(baseModule), pathToResolve)
}

For now you can probably go ahead and turn that option on.

It seems to me like that functionality should be tied to noCallThru instead of or at least in addition to noPreserveCache. I'll look into changing that.

bendrucker added a commit that referenced this issue Aug 21, 2018
Closes #215

* Existing test covered `@noCallThru`
* Module-wide `noCallThru()` should affect `_require`
@intervalia
Copy link
Author

I'll check it within the hour.

@intervalia
Copy link
Author

Yes. My bug is now gone. Thanks for the quick fix!!

bendrucker added a commit that referenced this issue Aug 21, 2018
Closes #215

* Existing test covered `@noCallThru`
* Module-wide `noCallThru()` should affect `_require`
allouis added a commit to allouis/Ghost that referenced this issue Mar 16, 2020
no-issue

This adds two new endpoints, one at /ghost/.well-known/jwks.json for exposing
a public key, and one on the canary api /identities, which allows the
Owner user to fetch a JWT.

This token can then be used by external services to verify the domain

* Added ghost_{public,private}_key settings

    This key can be used for generating tokens for communicating with
    external services on behalf of Ghost

* Added .well-known directory to /ghost/.well-known

    We add a jwks.json file to the .well-known directory which exposes a
    public JWK which can be used to verify the signatures of JWT's created
    by Ghost

    This is added to the /ghost/ path so that it can live on the admin
    domain, rather than the frontend. This is because most of its
    uses/functions will be in relation to the admin domain.

* Improved settings model tests

    This removes hardcoded positions in favour of testing that a particular
    event wasn't emitted which is less brittle and more precise about what's
    being tested

* Fixed parent app unit tests for well-known

    This updates the parent app unit tests to check that the well-known
    route is mounted. We all change proxyquire to use `noCallThru` which
    ensures that the ubderlying modules are not required. This stops the
    initialisation logic in ./well-known erroring in tests

thlorenz/proxyquire#215

* Moved jwt signature to a separate 'token' propery

    This structure corresponds to other resources and allows to exptend with
    additional properties in future if needed
allouis added a commit to TryGhost/Ghost that referenced this issue Mar 16, 2020
no-issue

This adds two new endpoints, one at /ghost/.well-known/jwks.json for exposing
a public key, and one on the canary api /identities, which allows the
Owner user to fetch a JWT.

This token can then be used by external services to verify the domain

* Added ghost_{public,private}_key settings

    This key can be used for generating tokens for communicating with
    external services on behalf of Ghost

* Added .well-known directory to /ghost/.well-known

    We add a jwks.json file to the .well-known directory which exposes a
    public JWK which can be used to verify the signatures of JWT's created
    by Ghost

    This is added to the /ghost/ path so that it can live on the admin
    domain, rather than the frontend. This is because most of its
    uses/functions will be in relation to the admin domain.

* Improved settings model tests

    This removes hardcoded positions in favour of testing that a particular
    event wasn't emitted which is less brittle and more precise about what's
    being tested

* Fixed parent app unit tests for well-known

    This updates the parent app unit tests to check that the well-known
    route is mounted. We all change proxyquire to use `noCallThru` which
    ensures that the ubderlying modules are not required. This stops the
    initialisation logic in ./well-known erroring in tests

thlorenz/proxyquire#215

* Moved jwt signature to a separate 'token' propery

    This structure corresponds to other resources and allows to exptend with
    additional properties in future if needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants