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

Npm.require submodule for packages loaded with Npm.depend() #3526

Closed
wants to merge 5 commits into from

Conversation

gsuess
Copy link
Contributor

@gsuess gsuess commented Jan 21, 2015

This fixes the following bug...

Steps to reproduce the bug:

Assert that amqplib is not installed: npm uninstall amqplib etc.

Create a package:

//package.js
Package.describe({
    summary: "Robust messaging for meteor that just works"
});

Npm.depends({
    "amqplib": "0.3.0"
});

Package.onUse(function (api) {
    api.use("underscore", "server");

    api.addFiles("amqplib.js", "server");
    api.export("AMQP");
});
//amqplib.js
console.log(Npm.require("amqplib/callback_api"));

Enabled it in an app and run it.

Expected Result:

{ connect: [Function: connect],
  credentials: { plain: [Function], external: [Function] } }

Actual Result:

W20150121-23:41:16.050(1)? (STDERR) /home/gsuess/WebstormProjects/meteor/dev_bundle/server-lib/node_modules/fibers/future.js:173
W20150121-23:41:16.050(1)? (STDERR)                                             throw(ex);
W20150121-23:41:16.051(1)? (STDERR)                                                   ^
W20150121-23:41:16.055(1)? (STDERR) Error: Can't find npm module 'amqplib/callback_api'. Did you forget to call 'Npm.depends' in package.js within the 'amqplib' package?
W20150121-23:41:16.056(1)? (STDERR)     at Object.Npm.require (/home/gsuess/WebstormProjects/keen-web/app/.meteor/local/build/programs/server/boot.js:149:17)
W20150121-23:41:16.056(1)? (STDERR)     at Package (packages/amqplib/amqplib.js:7:1)
W20150121-23:41:16.056(1)? (STDERR)     at /home/gsuess/WebstormProjects/keen-web/app/.meteor/local/build/programs/server/packages/amqplib.js:132:4
W20150121-23:41:16.056(1)? (STDERR)     at /home/gsuess/WebstormProjects/keen-web/app/.meteor/local/build/programs/server/packages/amqplib.js:141:3
W20150121-23:41:16.056(1)? (STDERR)     at /home/gsuess/WebstormProjects/keen-web/app/.meteor/local/build/programs/server/boot.js:207:10
W20150121-23:41:16.056(1)? (STDERR)     at Array.forEach (native)
W20150121-23:41:16.056(1)? (STDERR)     at Function._.each._.forEach (/home/gsuess/WebstormProjects/meteor/dev_bundle/server-lib/node_modules/underscore/underscore.js:79:11)
W20150121-23:41:16.056(1)? (STDERR)     at /home/gsuess/WebstormProjects/keen-web/app/.meteor/local/build/programs/server/boot.js:116:5

@avital avital mentioned this pull request Jan 23, 2015
@avital
Copy link
Contributor

avital commented Jan 23, 2015

Looks like this fixes #3505?

if (fs.existsSync(nodeModuleDir)) {
var nodeModuleDir = path.resolve(nodeModuleBase, name);

if (fs.existsSync(path.resolve(nodeModuleBase, name.split("/")[0]))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong? Why are we only looking at the first part of name.split("/")? And what if name has no slash in it?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think this looks like it could be right. Isn't the point that Npm.require("foo/bar") should be resolved by saying "if this package uses foo, then we should be trying to resolve "foo/bar" as within this module's foo"?

Node require has a bunch of different options; for example, require('foo/bar') can pull in a file that's called bar.js inside the foo package. Or that's called bar/index.js inside foo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If name has no slash then name.split('/')[0] === name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I now understand. Agreed.

@gsuess
Copy link
Contributor Author

gsuess commented Jan 23, 2015

@avital yes it does fix #3505.

@glasser glasser closed this in 8a39b84 Jan 24, 2015
@glasser
Copy link
Contributor

glasser commented Jan 24, 2015

Extended to cover another use case of Npm.require and merged. Thanks @gsuess for the fix and @hipertracker for the reproduction!

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.

None yet

3 participants