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

require-in-the-middle does not work when using request-promise & request-promise-native #61

Closed
kirrg001 opened this issue Mar 3, 2023 · 3 comments · Fixed by #63
Closed
Assignees
Labels
agent-nodejs Make available for APM Agents project planning.

Comments

@kirrg001
Copy link

kirrg001 commented Mar 3, 2023

Hi there!

I am running into an error when requiring request-promise & request-promise-native.

The request module is used by request-promise and request-promise-native. But require-in-the-middle returns a patched copy of request, which makes it impossible to require request-promise-native.

See https://github.com/request/request-promise-native/blob/master/lib/rp.js#L7
See https://github.com/request/request-promise/blob/master/lib/rp.js#L10

Although request is deprecated, I think this is a general problem with require-in-the-middle when using two libraries, which need a fresh copy of a module.

I have attached a zip, which reproduces the error.

Regards,
Kate

reproduce.zip

@kirrg001
Copy link
Author

kirrg001 commented Mar 6, 2023

One option to solve this: instead of returning the cached entry, you could return the original export if the export has changed.

You just need to move this line above the cache check and compare the export results.

const exports = self._origRequire.apply(this, arguments)

As the implementation of Node.js Module.prototype.require (=== Module._load) has it's own internal cache, it doesn't matter if you call the original require or not - see.

You would then check if the export have changed, if yes - return the original export.

@kirrg001
Copy link
Author

@trentm I've suggested a solution to the bug report. I can create a bug fix PR if you are okay with it. Thanks

@trentm trentm self-assigned this Mar 31, 2023
@trentm trentm added the agent-nodejs Make available for APM Agents project planning. label Mar 31, 2023
@trentm trentm added this to Planned in APM-Agents (OLD) via automation Mar 31, 2023
trentm added a commit that referenced this issue Apr 5, 2023
…odules

This allows a user to delete from `require.cache` to trigger a re-load
of a module (and a re-run of the hook's `onrequire`) on next `require()`.

This also removes the `.cache` field from the Hook object. It was never
documented, but it wasn't prefixed with `_` to hint at being private. As
well, some tests used it, which might have given the impression it could
be used externally. I'm calling this a semver-major change to give a
heads up to users that might have been using it.

Fixes: #61
@trentm
Copy link
Member

trentm commented Apr 5, 2023

@kirrg001 Sorry for the delay. I've created a PR that fixes this -- at least for the cases that I see, including your "reproduce.zip". Thanks very much for that.

trentm added a commit that referenced this issue Apr 12, 2023
…odules (#63)

This allows a user to delete from `require.cache` to trigger a re-load
of a module (and a re-run of the hook's `onrequire`) on next `require()`.

This also removes the `.cache` field from the Hook object. It was never
documented, but it wasn't prefixed with `_` to hint at being private. As
well, some tests used it, which might have given the impression it could
be used externally. I'm calling this a semver-major change to give a
heads up to users that might have been using it.

Fixes: #61
Obsoletes: #57
APM-Agents (OLD) automation moved this from Planned to Done Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
2 participants