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

Allow passing startDir to moduleconfig to avoid using cached module.parent #6

Conversation

m-kusnierz
Copy link

This package doesn't work correctly together with Opentelemetry packages.

In short, opentelemetry uses require-in-the-middle that has its own cache in addition to require cache which makes module.parent in moduleConfig.js#L9 to always resolve to the same file.

This issue has been described in open-telemetry/opentelemetry-js#3655. It has been fixed in require-in-the-middle v7.0.0, but the additional cache has been reintroduced in v7.1.1.

@@ -5,8 +5,13 @@ var cache = require("./cache"); //use requires caching to have a singleton
var getConfigPath = require("./getConfigPath"); //use requires caching to have a singleton
var path = require("path");

module.exports = function moduleConfig(paths, loadPathFunction) {
var startDir = path.dirname(module.parent.parent.filename);
module.exports = function moduleConfig(startDir, paths, loadPathFunction) {
Copy link
Collaborator

@bifrost bifrost Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make startDir optional and pass it as the last parameter - then it does not break existing usages

Suggested change
module.exports = function moduleConfig(startDir, paths, loadPathFunction) {
module.exports = function moduleConfig(paths, loadPathFunction, startDir) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a mess and I see that loadPathFunction is optional. Why not write your own module for moduleconfig?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we just wanted to avoid releasing this package under our scope.
@hilleer Do you have any thoughts on this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. whatever you find better/more suitable. Optimally I would simply pass an object to the function instead of optional arguments where the object has the optional options 🙂

@m-kusnierz
Copy link
Author

I'm closing this PR as we decided to implement these changes in forked repository for now.

@m-kusnierz m-kusnierz closed this Aug 17, 2023
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 this pull request may close these issues.

3 participants